Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API Proposal]: Modify Float/Double Equals and CompareTo to reflect -0.0 and +0.0 difference and ordering #73583

Closed
Tracked by #79053
mconnew opened this issue Aug 8, 2022 · 11 comments · Fixed by #75517
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Milestone

Comments

@mconnew
Copy link
Member

mconnew commented Aug 8, 2022

Background and motivation

IEEE 754 states that mathematical operations should consider -0.0 and +0.0 to be equal. The behavior of comparison methods such as .Equals and .CompareTo are not defined by IEEE 754. We already have precedent as .Equals and .CompareTo have different behavior than ==,>, and < when comparing NaN. Double.NaN == Double.NaN evaluates to false (as per IEEE 754), but Double.NaN.Equals(Double.NaN) evaluates to true. The CompareTo has similar behavior where it returns 0 to indicate they are the same.

Today if you have a list of doubles in an array which includes -0.0 and +0.0, calling Array<double>.Sort() is non-deterministic. The order of -0.0 and +0.0 will depend on the initial order of the array elements, Even with the same order, the resulting order is potentially fragile from one release of .NET to the next as the algorithm used for sorting is an internal implementation detail and could be changed between releases.

API Proposal

N/A as this is a proposal for a change in behavior with no public surface changes.

API Usage

N/A

Alternative Designs

An alternative is to provide new static properties on Float and Double which provide instances of IComparer<T> and IEqualityComparer<T> with this different behavior which can be provided to collections and method calls to order the zero values.

Risks

If code is consuming doubles or floats and using .Equals to compare them with 0.0, values which represent negative zero would now return false.
There will be a small performance regression in some cases as extra comparisons will need to be made. This should be able to be minimized with careful coding. We would only need to do a zero check if == returns true which will usually not be the case with floating point numbers.

@mconnew mconnew added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Aug 8, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Aug 8, 2022
@mconnew
Copy link
Member Author

mconnew commented Aug 8, 2022

@tannergooding, this is the issue I opened as a result of our discussion.

@ghost
Copy link

ghost commented Aug 9, 2022

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

IEEE 754 states that mathematical operations should consider -0.0 and +0.0 to be equal. The behavior of comparison methods such as .Equals and .CompareTo are not defined by IEEE 754. We already have precedent as .Equals and .CompareTo have different behavior than ==,>, and < when comparing NaN. Double.NaN == Double.NaN evaluates to false (as per IEEE 754), but Double.NaN.Equals(Double.NaN) evaluates to true. The CompareTo has similar behavior where it returns 0 to indicate they are the same.

Today if you have a list of doubles in an array which includes -0.0 and +0.0, calling Array<double>.Sort() is non-deterministic. The order of -0.0 and +0.0 will depend on the initial order of the array elements, Even with the same order, the resulting order is potentially fragile from one release of .NET to the next as the algorithm used for sorting is an internal implementation detail and could be changed between releases.

API Proposal

N/A as this is a proposal for a change in behavior with no public surface changes.

API Usage

N/A

Alternative Designs

An alternative is to provide new static properties on Float and Double which provide instances of IComparer<T> and IEqualityComparer<T> with this different behavior which can be provided to collections and method calls to order the zero values.

Risks

If code is consuming doubles or floats and using .Equals to compare them with 0.0, values which represent negative zero would now return false.
There will be a small performance regression in some cases as extra comparisons will need to be made. This should be able to be minimized with careful coding. We would only need to do a zero check if == returns true which will usually not be the case with floating point numbers.

Author: mconnew
Assignees: -
Labels:

api-suggestion, area-System.Runtime, untriaged

Milestone: -

@tannergooding tannergooding removed the untriaged New issue has not been triaged by the area owner label Aug 9, 2022
@tannergooding
Copy link
Member

tannergooding commented Aug 9, 2022

This is worth taking to API review for consideration.

IEEE 754 has a concept of "total order" and this is exposed in certain api's such as Max and Min. It also declares a totalOrder API which we don't expose.

The exact semantics it describes are:

  • -qNaN < -sNaN
  • -sNaN < x
  • -0 < +0
  • +sNaN > x
  • +qNaN > +sNaN

In .NET, we don't really have a concept of sNaN since we don't support IEEE 754 exceptions. Likewise, most of our NaN are normalized to -qNaN when doing constant folding or similar. So the most important concept here is really -0 < +0 and most users won't see a change in behavior for NaN unless they go and create their own, but it would be a visible change in some cases.

==============

If API review deems this change would be too breaking, we should investigate what alternatives we can provide to help provide a concept of overall ordering, such as a TotalOrder and TotalOrderMag method or some other alternatives.

Several languages provide a concept of strong vs partial ordering. Integers are strongly ordered while IEEE 754 uses a partial ordering by default. .NET exposes a concept of strong ordering on top of IEEE 754 semantics via Equal and CompareTo, but it doesn't match the IEEE 754 semantics.

@tannergooding tannergooding added api-ready-for-review API is ready for review, it is NOT ready for implementation breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Aug 9, 2022
@tannergooding tannergooding added this to the 8.0.0 milestone Aug 12, 2022
@Hafthor
Copy link
Contributor

Hafthor commented Aug 30, 2022

Q: Within the space of, say -qNaNs, are they all expected to sort as equals? or do we sort by their significands?

@tannergooding
Copy link
Member

The IEEE 754 TotalOrder views them as equal and leaves payload consideration as "implementation defined".

@jkotas
Copy link
Member

jkotas commented Aug 30, 2022

The IEEE 754 TotalOrder views them as equal and leaves payload consideration as "implementation defined".

It means that the non-deterministic behavior of sorting is not going to be fully fixed by this proposal.

Alternative Designs

Another alternative design to switch to a stable sorting algorithm in the BCL that would fix this whole class of problems for good.

@bartonjs
Copy link
Member

bartonjs commented Sep 6, 2022

Video

  • Rather than take a breaking change here, it seems less confusing to just expose a new comparer for the concept.
namespace System.Numerics;

public struct TotalOrderIeee754Comparer<T> : IComparer<T>, IEqualityComparer<T>
    where T : IFloatingPointIeee754<T>
{
    // all the methods required by those interfaces
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. api-ready-for-review API is ready for review, it is NOT ready for implementation labels Sep 6, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Sep 13, 2022
@teo-tsirpanis
Copy link
Contributor

Shouldn't we also add an Instance static property?

@tannergooding
Copy link
Member

We opted for no Instance static proeprty in API review and that we could add one in the future with enough requests.

@teo-tsirpanis
Copy link
Contributor

Oh, it's a struct, didn't see.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Sep 24, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants