Use `DoubleComparions` and other DH-specific comparisons for UpdateBy RollingMin/Max, EmMin/Max, CumMin/Max

deephaven / deephaven-core

Deephaven Community Core

Other

251 stars 80 forks source link

Use `DoubleComparions` and other DH-specific comparisons for UpdateBy RollingMin/Max, EmMin/Max, CumMin/Max #3818

Open lbooker42 opened 1 year ago

lbooker42 commented 1 year ago

In contrast to aggBy.aggMin/aggMax, UpdateBy min/max operators use Math.min/Math.max for comparison. This may lead to computation differences in these functions when provided identical datasets.

Correct UpdateBy operators to use the Deephaven comparison classes and unify the implementation of min/max in aggregating operators.

devinrsmith commented 1 year ago

I think there's some ambiguity, as it's defined right now, how one properly implements "min" / "max" using the existing operators that DoubleComparisons presents. We should be sure that we are clear in our definitions, and have unit tests as well. I'm hoping to follow up w/ a new DoubleComparisons ticket shortly.

lbooker42 commented 1 year ago

As a note to the implementer, the rolling and cumulative min/max algorithms are verified in unit testing through the Numerics library. However, this library is not internally consistent:

cummin() uses Math.min() but min() simply uses < for comparison. Neither leverages the comparison classes.
cummax() uses CompareUtils#compare() which uses < and > for determination while max() uses >