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.

Sign up for GitHub

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

Jump to bottom

Expose Math.MaxNumber and Math.MinNumber functions that don't propagate NaN #29279

Closed
tannergooding opened this issue Apr 16, 2019 · 14 comments
Closed

Expose Math.MaxNumber and Math.MinNumber functions that don't propagate NaN #29279

tannergooding opened this issue Apr 16, 2019 · 14 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Numerics
Milestone
Future

Comments

@tannergooding
Copy link
Member

Rationale

dotnet/coreclr#20912 updated the existing Math.Min/Math.Max functions to be both IEEE 754:2008 compliant and to be inline with the C/C++ language standard (Annex F - IEC 60559 floating-point arithmetic). This also brought it inline with MSVC, GCC, and Clang for their implementations under 'precise' floating-point mode.

However, it looks like the draft for IEEE 754:2019 (a summary can be found here http://754r.ucbtest.org/background/, but I also have the latest draft locally) is changing things up a bit. Namely, they are removing minNum/maxNum from the list of "required" operations and replacing them with new minimum/maximum and minimumNumber/maximumNumber operations which are recommended by not required and which more clearly specify various behaviors.

  • minimumNumber/maximumNumber are largely compatible with the existing minNum/maxNum functions. But also clarifies that +0 is greater than -0 for the purpose of this function and clarifies the behavior of signalling NaN.
  • minimum/maximum are new and would propagate the NaN as expected here. They likewise clarify that +0 is greater than -0 for the purpose of this function.

Proposal

Given this change of behavior in the new spec, it might be desirable to revert Math.Min/Math.Max to continue propagating the NaN result. We could then expose new functions which do not propagate the NaN instead.

This would be, overall, a more compatible change and would still allow us to be IEEE compliant.

Such new APIs would be:

public static partial class Math
{
    public static double MaxNumber(double x, double y);
    public static double MaxMagnitudeNumber(double x, double y);
    public static double MinNumber(double x, double y);
    public static double MinMagnitudeNumber(double x, double y);
}

public static partial class MathF
{
    public static float MaxNumber(float x, float y);
    public static float MaxMagnitudeNumber(float x, float y);
    public static float MinNumber(float x, float y);
    public static float MinMagnitudeNumber(float x, float y);
}

Notes

WPF hit a bug due to the change in behavior: dotnet/wpf#521

We have detected a perf regression due to the new behavior being more complex: https://github.com/dotnet/coreclr/issues/22951. We still need to keep the change that +0 is greater than -0, but it will lessen the regression overall and make a fix easier to attain.

IEEE 754:2008 refer to these as minNum, maxNum, minNumMag, and maxNumMag. However, IEEE 754:2019 (draft) refers to them as minimumNumber, maximumNumber, minimumMagnitudeNumber, and maxmimumMagnitudeNumber. I opted with the latter postfix (MagnitudeNumber, rather than NumberMagnitude), as I believe it is more readable.
@tannergooding
Copy link
Member Author

cc. @jkotas, @danmosemsft.

Thoughts?

@jkotas
Copy link
Member

jkotas commented Apr 16, 2019

Reverting Math.Max/Min to have the original behavior sounds fine to me.

I do not think that the IEEE compliance for these operations is important, especially given that the IEEE is changing their opinion about them as well.

@tannergooding
Copy link
Member Author

I do not think that the IEEE compliance for these operations is important, especially given that the IEEE is changing their opinion about them as well.

It's not that they are changing their opinion as the original functionality for minNum and maxNum remains the same, with an additional clarification that +0 should be greater than -0. Instead, they are now exposing additional functions minimum and maximum that don't propagate NaN based on common implementations and user feedback.

@jkotas
Copy link
Member

jkotas commented Apr 16, 2019

api-ready-for-review

I do not think we should be adding new APIs in 3.0. Just revert the Math.Max/Min for 3.0, and leave the rest for future.

@tannergooding
Copy link
Member Author

I do not think we should be adding new APIs in 3.0

That would mean we are no longer implementing all IEEE 754:2008 required general operations for 3.0.

I think the best scenario is to:

  • Partially revert Math.Max/Math.Min so that they propagate the NaN (as previous); but continue comparing +0 as greater than -0.
  • Take the new non-propagating functionality we have already exposed and move it into the new methods I gave above.

This ensures we remain IEEE 754:2008 compliant, ensures that we are not breaking WPF for a scenario they are already depending on, keeps the functionality deterministic (regardless of the ordering of the inputs), and moves us to be inline with IEEE 754:2019.

@jkotas
Copy link
Member

jkotas commented Apr 16, 2019

That would mean we are no longer implementing all IEEE 754:2008 required general operations for 3.0.

I do not see a problem with it. This is corner case functionality that it fine to mention in the fine print.

We need to get into the shipping mindset for 3.0. Work through the bugs and stop faulting in more nice-to-haves.

@tannergooding
Copy link
Member Author

I do not see a problem with it. This is corner case functionality that it fine to mention in the fine print.

Being able to say that we are IEEE compliant for required operations is something that can be quite important for various numerics libraries, for porting code from native, and for determinism across platforms.

We need to get into the shipping mindset for 3.0. Work through the bugs and stop faulting in more nice-to-haves.

I agree, but if we can fix a "bug" by moving the new functionality to a neighboring function and the code has already been in/tested for many months now; then I think that is better than pulling it out completely. Especially when it allows us to tick a compliance checkbox that has been unchecked for years and include that as part of the 3.0 milestones we reached.

The alternative is that this is pulled out in 3.0; and then reviewed/approved and checked back in for vNext (likely 3.1) which seems to be a worse option considering how trivial the code is.

@tannergooding
Copy link
Member Author

I'll move this to Future for now, but I think that we should consider this for 3.0 as part of the bug fix as it would improve the release story for 3.0 at a trivial cost.

@danmoseley
Copy link
Member

I am happy with pushing out this new API and documenting the omission. Thank you @tannergooding

@jkotas
Copy link
Member

jkotas commented Apr 16, 2019

move this to Future for now

Reverting the behavior of Math.Max/Min should stay in 3.0.

@tannergooding
Copy link
Member Author

Reverting the behavior of Math.Max/Min should stay in 3.0.

I'll open a separate issue to track reverting the behavior to propagate the NaN.

@tannergooding tannergooding changed the title Behavior change for Math.Min and Math.Max Expose Math.MaxNumber and Math.MinNumber functions that don't propagate NaN Apr 16, 2019
@tannergooding
Copy link
Member Author

Opened: https://github.com/dotnet/corefx/issues/36931

@GrabYourPitchforks
Copy link
Member

Feedback: Should we have a wider discussion about putting future APIs on the primitive types (double, half, etc.) instead of the Math / MathF / MathH classes? Otherwise there's likely to be an explosion of Math* types and APIs on those types.

If we decide that putting these APIs on the primitive types isn't worthwhile then the proposal as stated here seems viable. There was some discussion on whether we should keep the Number name and whether those APIs would be discoverable enough, but: (a) these names match what IEEE uses; and (b) it's ok if users keep using the old APIs instead of the new APIs for the majority of use cases.

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 5.0 milestone Feb 1, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@tannergooding tannergooding removed the untriaged New issue has not been triaged by the area owner label Mar 5, 2020
@tannergooding tannergooding modified the milestones: 5.0.0, Future Jun 23, 2020
@tannergooding
Copy link
Member Author

Closing in favor of #20137

@ghost ghost locked as resolved and limited conversation to collaborators Feb 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Numerics
Projects
None yet
Milestone
Future
Development

No branches or pull requests
6 participants
@GrabYourPitchforks @maryamariyan @danmoseley @jkotas @msftgits @tannergooding