Fix comparison operators for double values in DS

[aparajit-pratap]

Purpose

Fixes equality and comparison operators in DesignScript for double type.
Github issue: #9733

Main points addressed in this PR:

• DS comparison operators <, >, <=, >= use the underlying platform C# operators
• Equality operators == and != are left unchanged and continue to use a DS level tolerance of 1e-5 ni order to prevent breaking changes

Declarations

Check these if you believe they are true

• The code base is in a better state after this PR
• Is documented according to the standards
• The level of testing this PR includes is appropriate
• User facing strings, if any, are extracted into *.resx files
• All tests pass using the self-service CI.
• Snapshot of UI changes, if any.
• Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@mjkkirschner

FYIs

@DynamoDS/dynamo

[mjkkirschner]

hmm, I'm not sure why this ever had a tolerance like this?
I guess this better aligns to other OOTB double equality comparison in python, c# etc?

We should definitely consider though what this might break - it seems to me that it's worth it though because some of these results very confusing to users who use a > 5 digits of precision - this doesn't seem like a very uncommon use case to me regarding optimization or when dealing with parameter spaces over a curve etc.

[mjkkirschner]

@jnealb @saintentropy @Dewb FYI

[aparajit-pratap]

Something simple like this fails now after this change:

[jnealb]

Jira task for this? We need a math epic/story to centralize the issues.

[aparajit-pratap]

For the time being I have adjusted the tolerance to make it smaller so that floating point comparisons will be possible to higher decimal places in DS however I feel we should make it settable by the user via a UI to give more flexibility and control. Thoughts?

[mjkkirschner]

@aparajit-pratap - have you considered adding an overload like: Double.Equals(other, epsilon)? - We could add this method and leave the current implementation as is to avoid changing current behavior - and then document the current behavior.

It does feel odd to have an arbitrary tolerance when considering large scale projects (kilometers) or non geometrical comparisons - fitness functions etc - I am curious what @RyaPorter was trying to do when they ran into this issue?

[ColinDayOrg]

One thing to possibly consider; one of the intentions of the Dynamo Primitives package was to be able to "grid" up a double space (in 1D, 2D, and 3D), and once the space is gridded then integers can be used instead of doubles which gets rid of the need for epsilons. Using epsilons is necessary in double comparisons, but will inevitably lead to tolerance creep. Just something to consider if it could be useful.

[aparajit-pratap]

@aparajit-pratap - have you considered adding an overload like: Double.Equals(other, epsilon)? - We could add this method and leave the current implementation as is to avoid changing current behavior - and then document the current behavior.

@mjkkirschner how will this overload be used? How is Double.Equals(other, epsilon) different or advantageous over Math.Abs(lhs - rhs) < Tolerance? Will epsilon be user input or the standard Double.Epsilon?

[aparajit-pratap]

@gregmarr do you have any inputs on what should be the best way to implement equality comparisons for floating point values in languages?

[aparajit-pratap]

Hmm, I see this is an issue in Thunderstorm as well:

[Dewb]

A useful reference for anyone who isn't already familiar with it:
What Every Computer Scientist Should Know About Floating-Point Arithmetic, by David Goldberg
http://docs.oracle.com/cd/E19957-01/806-3568/ncg_goldberg.html

This might also be relevant:
https://possiblywrong.wordpress.com/2013/11/15/floating-point-equality-its-worse-than-you-think/

Doing equality tests on IEEE floating point numbers pretty much never results in what the user intends. The Equals node in the Dynamo environment ought to have an epsilon clearly communicated to the user, whether it's configurable or not. The DS == operator is trickier, I think it would be reasonable for it to behave in the same way as the underlying implementation language's == operator does, and users should therefore avoid using it for floating-point numbers, preferring a separate library function with an epsilon value.

But the complaint in #9733 is about the < operator, which should have stable, correct results for doubles. How does this involve a boolean comparison function that uses == instead of the -1/0/+1 comparison test function?

[mjkkirschner]

@Dewb in this case it appears the < and > operators are checking for !equals() as well - I guess to implement <= or >=

[Dewb]

@mjkkirschner where is that in the code?

[Dewb]

Ah, I see:

Dynamo/src/Engine/ProtoCore/Utils/MathUtils.cs

Line 11 in 7b3642f

return (lhs < rhs) && !Equals(lhs, rhs);

Dynamo/src/Engine/ProtoCore/Lang/BuiltInFunctionEndPoint.cs

Line 2601 in fa31e0e

if (Equals(sv1, sv2))

The Equals call ought to be eliminated from all of these methods. They can be implemented with calls to Double.CompareTo(Double) or Int.CompareTo(Int) as appropriate (with casting if the arguments aren't the same numeric type.)

For StackValueComparerForDouble inBuiltInFunctionEndPoint.cs, it might also be a good idea to consider removing the flexible ascending/descending comparison flag in the constructor, as it doesn't appear to be used anywhere and complicates the implementation.

In C-like languages, the convention is that a method named Compare called on a and b returns <0 if a < b, 0 if a == b, and >0 if a > b. Any DS VM methods named Compare probably ought to follow this convention, without flags to reverse the behavior. Along the same lines, the existing Compare* methods that currently return bool ought to be renamed in terms of Equals or IsEqual, etc.

[Dewb]

Hmm, I see this is an issue in Thunderstorm as well:

@aparajit-pratap This is the behavior that I would expect in an environment that uses IEEE floats. You'll observe the same results in C#, Javascript, Go, and Python.

[aparajit-pratap]

@aparajit-pratap This is the behavior that I would expect in an environment that uses IEEE floats. You'll observe the same results in C#, Javascript, Go, and Python.

@Dewb so in that case are you suggesting that it's okay to implement the Equals method in:

Dynamo/src/Engine/ProtoCore/Utils/MathUtils.cs

Line 29 in 351c88b

public static bool Equals(double lhs, double rhs)

as lhs.Equals(rhs)?

[Dewb]

No, any floating-point equality method that uses a tolerance value should be implemented that way (comparing the absolute value of the difference to the tolerance value.)

I'm saying that any methods implementing less than or greater than on floating point values should use either the built-in operators or the built-in CompareTo method, and should not call built-in or custom Equals methods, use the == operator, or check a tolerance.

[aparajit-pratap]

No, any floating-point equality method that uses a tolerance value should be implemented that way (comparing the absolute value of the difference to the tolerance value.)

@Dewb I got your point on removing the Equals check from the < and > operator implementations. I was specifically asking about the == operator implementation.

From your comment about languages like C# I understood that for the equality check it is okay to use the underlying equality of the language we're implementing with (C# in this case) and we could leave it up to users to do "comparing the absolute value of the difference to the tolerance value" for floating point values in their graphs themselves OR have a separate node that expects an epsilon input additionally. On the other hand if we wish to continue to have the tolerance check implicit in the node, the question is on what basis do we select the tolerance. The current tolerance value of 1e-5 seems too high.

Also in TS the equality operator simply uses the equality check of the language (Go): https://git.autodesk.com/aecgd/thustrm/blob/c015d689975b9e9a99711ca2f955eb82962d8c93/intrinsics/intrinsics.go#L845

[aparajit-pratap]

It looks like the only difference between Double.CompareTo vs. the builtin < and > operators is the added support for 0.0d vs -0.0d and Nan numbers: https://www.tutorialspoint.com/java/lang/double_compareto.htm

[aparajit-pratap]

I have left the == operator implementation untouched for now (at the cost of Dynamo vs. TS behavior still being different) and fixed the comparison operators as per Mike D's suggestions. This should address #9733 mostly. Please give it one more look.

[mjkkirschner]

@aparajit-pratap lets just change the name of this PR (since it address comparison operators only) and then LGTM.

[mjkkirschner]

one more request - is there a test which checks the operators work for higher precision doubles (over e^-5)?

[Dewb]

My comments earlier were a little meandering. It distills down to one rule: we shouldn't be in the business of re-implementing the logic of any floating-point operations. We should always use the standard operators (or .CompareTo when we need the combined comparison operation.)

The only thing we should offer to augment that featureset is an explicitly toleranced "sufficiently close" or "sufficiently different" method which should be distinct from the == operator.

What we do in the node library is a slightly different question, but script should definitely follow the expectations and behavior for IEEE doubles.

[Dewb]

It looks like the only difference between Double.CompareTo vs. the builtin < and > operators is the added support for 0.0d vs -0.0d and Nan numbers: https://www.tutorialspoint.com/java/lang/double_compareto.htm

Sorry, I didn't mean to imply that we should prefer CompareTo over operators, but that we should use CompareTo when we wanted the <0 / 0 / >0 combined comparison operation.

[aparajit-pratap]

I have now removed any custom implementation for comparison operators except for ==. However, it still seems odd to me that we now have a custom implementation for only MathUtils.Equals that uses a tolerance. So now both these statements are true, which is confusing:

a = 0.3;
b = 0.2;
c = a - b < 0.1;    // c is true
d = a - b == 0.1; // d is true

I would therefore propose that we do not have a custom implementation for == operator in DS either, but simply use the underlying == available in C#. This will also make this behavior consistent with Thunderstorm. This will encourage users to follow best practices for floating point equality checks by supplying their own tolerance and make them write DS code like this:

c = Math.Abs(a - b) < user_tolerance;

In addition we could deprecate the existing == node and instead add a new node that takes 3 inputs - lhs (double), rhs (double), and tolerance. I think this proposal also aligns with @Dewb's thoughts if I understand correctly where he's coming from.

[mjkkirschner]

so to sum up the changes so far:

• comparison is now done using the underlying platform comparison
• == still uses a tolerance to avoid breaking changes

your proposal in the last comment sounds reasonable, but like we talked about earlier I'm not sure how to gauge the number of graphs this would break. I'm still leaning towards adding the Equals with tolerance method and changing the existing behavior at a major version change.

Can we think of some way of estimating if the impact of this change will break many existing graphs?

[aparajit-pratap]

@mjkkirschner yes, you have summarized these changes correctly.
Yes, we can add a new "Equals" node with tolerance and keep the existing behavior of == operator in order to prevent breaking graphs until the next major release. However, note there are changes here that also could break existing behavior of <, >, etc. operators. Something like this was at least consistent before this change but is now inconsistent:

a = 0.3;
b = 0.2;
c = a - b < 0.1;    // false before, true now
d = a - b == 0.1; // true 

I would therefore recommend either deferring this whole PR for later or adding the change for equality as well, which is to simply use the platform ==. I'll schedule a meeting for us to discuss next week.

[mjkkirschner]

@gregmarr we have a meeting tomorrow on this to discuss pros/cons / breaking changes - do you want to attend?