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

[Proposal] Expose additional MidpointRounding modes #27205

Closed
tannergooding opened this issue Aug 23, 2018 · 12 comments · Fixed by dotnet/coreclr#20815 or dotnet/corefx#33297
Closed

[Proposal] Expose additional MidpointRounding modes #27205

tannergooding opened this issue Aug 23, 2018 · 12 comments · Fixed by dotnet/coreclr#20815 or dotnet/corefx#33297
Labels
api-approved API was approved in API review, it can be implemented area-System.Numerics help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@tannergooding
Copy link
Member

Rationale

The IEEE 754:2008 spec calls out five different supported "rounding directions", 4 of which are required for binary-floating point values.

CoreFX currently exposes only 1 of the required rounding directions and the 1 optional rounding mode. We should expose the remaining three rounding directions in order to be more compliant and give users the greatest flexibility when writing their applications.

Proposed API

public partial enum MidpointRounding
{
    ToPositiveInfinity, // `roundTowardPositive` - the result closest to and no less than the infinitely precise result
    ToNegativeInfinity, // `roundTowardNegative` - the result closest to and no greater than the infinitely precise result
    ToZero              // `roundTowardZero` - the result closest to and no greater in magnitude than the infinitely precise result
}
@tannergooding
Copy link
Member Author

@tannergooding
Copy link
Member Author

This will require additional work in Math.Round, MathF.Round, and Decimal.Round.

@tannergooding
Copy link
Member Author

CC. @eerhardt, @danmosemsft

@hamish-rose
Copy link
Contributor

Hello

I would like to contribute to corefx - May I take this one ?

@danmoseley
Copy link
Member

For sure you can @hamish-rose. I sent you a collaborator link. If you accept, I can sign this to you. Note when you join it defaults to subscribing you to all repo activity, you will profligate want to switch that off as this repo is busy.

@danmoseley
Copy link
Member

danmoseley commented Oct 10, 2018

Please ask here if you have questions. In the documentation folder in this repo is everything you should need to get started.

@hamish-rose
Copy link
Contributor

Thanks, I've accepted.

@pentp
Copy link
Contributor

pentp commented Feb 12, 2019

These are not midpoint rounding modes but directed rounding modes.
Naming them as Truncate/Floor/Ceiling would be more consistent with the existing Math class API names.

@tannergooding
Copy link
Member Author

This was discussed in the API review and it was determined that we should still expose them on MidpointRoundingMode rather than introducing a new type (such as DirectedRoundingMode).

CC. @terrajobst who may have the notes from that meeting?

@tannergooding
Copy link
Member Author

I'm not opposed to having separate overloads that take a DirectedRoundingMode enum, but we would need to take this back through API review and update the corresponding PRs.

@pentp
Copy link
Contributor

pentp commented Feb 12, 2019

I'm still concerned about naming - it's really confusing to see MidpointRounding.AwayFromZero and MidpointRounding.ToPositiveInfinity next to each other when they actually mean completely different things (at first look, one would assume that for positive numbers they would behave identically).

I propose renaming them to Truncate/Floor/Ceiling to avoid such confusion.

@tannergooding
Copy link
Member Author

I propose renaming them to Truncate/Floor/Ceiling to avoid such confusion.

I don't think Truncate/Floor/Ceiling will help avoid any of the confusion here. The names mean effectively the same thing either way, the "confusion" comes from it being on MidpointRounding when they are in-fact DirectedRounding modes.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 15, 2020
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.Numerics help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
5 participants