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

Tracking Issue for round_ties_even #96710

Closed
3 tasks done
Jules-Bertholet opened this issue May 4, 2022 · 19 comments · Fixed by #120150
Closed
3 tasks done

Tracking Issue for round_ties_even #96710

Jules-Bertholet opened this issue May 4, 2022 · 19 comments · Fixed by #120150
Labels
A-floating-point Area: Floating point numbers and arithmetic C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Jules-Bertholet
Copy link
Contributor

Jules-Bertholet commented May 4, 2022

Feature gate: #![feature(round_ties_even)]

This is a tracking issue for the round_ties_even feature, which adds a new method to f32 and f64 for rounding to the nearest integer, with ties rounded to the nearest even number.

Public API

impl f32 {
    fn round_ties_even(self) -> f32;
}

impl f64 {
    fn round_ties_even(self) -> f64;
}

Steps / History

Unresolved Questions

  • Naming: round_ties_even, round_even, round_to_even, round_banker... many ways to paint this bikeshed
  • Should the behavior of the round function be changed to this?
  • Is it okay to use rint in the implementaton? (see Add round_ties_even to f32 and f64 #95317 (comment))
@Jules-Bertholet Jules-Bertholet added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels May 4, 2022
@Miksel12
Copy link

Miksel12 commented Aug 19, 2022

I think round should be changed to round ties to even, both x86 and ARM use it as the default rounding mode and IEEE 754 states it should be the default. It is also the mode with the least amount of bias.
As for round ties away from zero, which is the current mode used for round, it is actually not required by IEEE 754 for binary representation and is thus not implemented by ARM and x86. Which makes the current round pretty slow: #55107.
I don't know why round ties away from zero was chosen but it is pretty clear it is the wrong default.
If round was to be changed to ties to even, then the four rounding modes (round, ceil, floor and trunc) would match the required IEEE rounding modes and the rounding implemented by x86 and ARM (and probably most other ISA's, though RISC-V supports round ties away from zero).

@ajtribick
Copy link
Contributor

From the perspective of maintaining a crate that implements a custom floating point type that tries to provide a similar API to the built-in f32/f64, I'd be somewhat against changing round from its documented behavior at this point (although I'd agree it isn't ideal as a choice for the default round function). It might also be worth getting some opinion from the num folks on this as they have also implemented round on their rational number types as rounding ties away from zero, similarly the documentation of round on the Float/FloatCore in num_traits.

My suspicion for why away-from-zero was chosen is that it matches how round is specified to operate in C (and similarly std::round in C++), e.g. N1570 section 7.12.9.6:

The round functions round their argument to the nearest integer value in floating-point format, rounding halfway cases away from zero, regardless of the current rounding direction.

I definitely think having a function with the round ties to even mode should be available though. My preference would be round_ties_even over round_even (because to me the latter sounds more like rounding to an even number), but it's not a particularly strong preference there.

@workingjubilee workingjubilee added the A-floating-point Area: Floating point numbers and arithmetic label Jan 27, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 7, 2023
…=pnkfelix,m-ou-se,scottmcm

Add `round_ties_even` to `f32` and `f64`

Tracking issue: rust-lang#96710

Redux of rust-lang#82273. See also rust-lang#55107

Adds a new method, `round_ties_even`, to `f32` and `f64`, that rounds the float to the nearest integer , rounding halfway cases to the number with an even least significant bit. Uses the `roundeven` LLVM intrinsic to do this.

Of the five IEEE 754 rounding modes, this is the only one that doesn't already have a round-to-integer function exposed by Rust (others are `round`, `floor`, `ceil`, and `trunc`).  Ties-to-even is also the rounding mode used for int-to-float and float-to-float `as` casts, as well as float arithmentic operations. So not having an explicit rounding method for it seems like an oversight.

Bikeshed: this PR currently uses `round_ties_even` for the name of the method. But maybe `round_ties_to_even` is better, or `round_even`, or `round_to_even`?
bjorn3 pushed a commit to bjorn3/rust that referenced this issue Mar 15, 2023
…=pnkfelix,m-ou-se,scottmcm

Add `round_ties_even` to `f32` and `f64`

Tracking issue: rust-lang#96710

Redux of rust-lang#82273. See also rust-lang#55107

Adds a new method, `round_ties_even`, to `f32` and `f64`, that rounds the float to the nearest integer , rounding halfway cases to the number with an even least significant bit. Uses the `roundeven` LLVM intrinsic to do this.

Of the five IEEE 754 rounding modes, this is the only one that doesn't already have a round-to-integer function exposed by Rust (others are `round`, `floor`, `ceil`, and `trunc`).  Ties-to-even is also the rounding mode used for int-to-float and float-to-float `as` casts, as well as float arithmentic operations. So not having an explicit rounding method for it seems like an oversight.

Bikeshed: this PR currently uses `round_ties_even` for the name of the method. But maybe `round_ties_to_even` is better, or `round_even`, or `round_to_even`?
antoyo pushed a commit to antoyo/rust that referenced this issue Jun 19, 2023
…=pnkfelix,m-ou-se,scottmcm

Add `round_ties_even` to `f32` and `f64`

Tracking issue: rust-lang#96710

Redux of rust-lang#82273. See also rust-lang#55107

Adds a new method, `round_ties_even`, to `f32` and `f64`, that rounds the float to the nearest integer , rounding halfway cases to the number with an even least significant bit. Uses the `roundeven` LLVM intrinsic to do this.

Of the five IEEE 754 rounding modes, this is the only one that doesn't already have a round-to-integer function exposed by Rust (others are `round`, `floor`, `ceil`, and `trunc`).  Ties-to-even is also the rounding mode used for int-to-float and float-to-float `as` casts, as well as float arithmentic operations. So not having an explicit rounding method for it seems like an oversight.

Bikeshed: this PR currently uses `round_ties_even` for the name of the method. But maybe `round_ties_to_even` is better, or `round_even`, or `round_to_even`?
@tgross35
Copy link
Contributor

tgross35 commented Aug 9, 2023

Could this do something like use roundeven when available but fallback to rint when it isn't?

@tjallingt
Copy link

tjallingt commented Jan 3, 2024

I hope this is appropriate/helpful, I would like this feature stabilized so I will attempt a stabilization report.
I can also make a stabilization PR but it seems FCP should happen before that.

Request for Stabilization

I think the round_ties_even methods should be stabilized in version 1.77.0 (beta on 2 February, stable on 21 March).

Implementation history

These methods have been added in #95317 (merged 7 March, 2023) which corresponds to the 1.70 timeframe (branched on 14 April, 2023).

API Summary

The round_ties_even feature will be stabilized wich adds the following methods to std:

impl f32 {
    fn round_ties_even(self) -> f32;
}

impl f64 {
    fn round_ties_even(self) -> f64;
}

Experience reports

These methods are used in several projects:

There are also references to this feature/tracking issue on manual implementations of round_ties_even which are waiting for this to be stabilized:

Documentation

Both methods have been documented in the std rustdoc

Unresolved questions

The following questions are still unresolved:

  1. Naming: round_ties_even, round_even, round_to_even, round_banker... many ways to paint this bikeshed
  2. Should the behavior of the round function be changed to this?
  3. Is it okay to use rint in the implementaton? (see Add round_ties_even to f32 and f64 #95317 (comment))

I will try to answer these questions

1. Naming

No further comments have been made about the naming of these method. round_ties_even seems to be a clear, descriptive name for this functionality. This name also places it close to round in the docs to ensure it will be discoverable.

2. Changing round

While the default behaviour of round is not ideal it seems to be consistent with C and C++. Changing the behaviour of round would likely be a difficult and lengthy process and would involve adding round_ties_zero methods (I believe). This could be done as a followup to stabilizing round_ties_even if desired but, in my opinion, should not block the stabilization of this feature given that this rounding mode is currently not available in Rusts standard library.

3. Use rint

Not only does Rust assume the default rounding mode, so do the LLVM floating-point intrinsics that back it.

source

As you said though, Rust already assumes the default rounding mode, so it doesn't matter too much. I think roundeven is the "more correct" choice to use but it's not well supported so it's OK to just use rint for now. Ideally we should support both in libm.

source

From these comments it appears that using rint poses no immediate problems. Rust already assumes the default rounding mode is used. It seems safe to extend that assumption to round_ties_even.

There does seem to be agreement that roundeven should be preferred once it is well supported.
I don't have enough knowledge to say for sure but; it seems likely that chaning from using rint to roundeven should be backwards compatible and therefore should not block the stabilization of this feature.

@tjallingt
Copy link

I'm not sure the previous post will be seen by anyone without a ping/mention.
I hope it is okay to mention you @m-ou-se I hope I'm following the right process 😅

@RalfJung RalfJung added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Jan 5, 2024
@RalfJung
Copy link
Member

RalfJung commented Jan 5, 2024

I've nominated this for libs-api discussion, so the team should take a look as part of their triage.

I don't have enough knowledge to say for sure but; it seems likely that chaning from using rint to roundeven should be backwards compatible and therefore should not block the stabilization of this feature.

Correct; as far as Rust is concerned, these two are equivalent.

@RalfJung
Copy link
Member

RalfJung commented Jan 6, 2024

Regarding the point of changing round, note that today round is consistent with float-to-int casts. That also seems like an argument against changing it.

EDIT: no I'm completely wrong, float2int rounds towards zero, it doesn't even use "nearest" semantics. oops

@Amanieu
Copy link
Member

Amanieu commented Jan 9, 2024

We discussed this in the libs-api meeting today. This function exposes the last IEEE 754 rounding mode that isn't yet accessible through other functions on floats.

We decided against changing the behavior of the existing round method: this would be a breaking change, and round-ties-to-even is not the obvious behavior that most people would expect.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Jan 9, 2024

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jan 9, 2024
@rfcbot
Copy link

rfcbot commented Jan 9, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@scottmcm
Copy link
Member

scottmcm commented Jan 9, 2024

I do think it would be nice for the simple round to be banker's, even if it's not the initial expectation, like how .. is half-open despite many people expecting inclusive. But not making a breaking change sounds like the right choice since we don't have a time machine.

@joshtriplett
Copy link
Member

@scottmcm I do wish we had an obvious short name we could give this, because it's unfortunate that the thing you probably should use most of the time (since among other things it's supported in hardware) has a name that's more than twice the length. But it's not obvious what name we could use for that that wouldn't be obfuscating.

@scottmcm
Copy link
Member

scottmcm commented Jan 9, 2024

Agreed, Josh. But I don't have any smart ideas for anything better than the current proposal 🙁

@kalcutter
Copy link

We decided against changing the behavior of the existing round method: this would be a breaking change, and round-ties-to-even is not the obvious behavior that most people would expect.

What do you claim to be the "obvious" behavior that people should expect? Round-ties-to-even is the default IEEE-754 rounding mode and is used by Python which is the most popular programming language by some metrics. Other popular languages like Javascript and C++ do not even agree on how negative numbers should be rounded.

Would it be possible to change the behavior of round starting with the 2024 edition?

@Jules-Bertholet
Copy link
Contributor Author

Rounding ties away from zero is the rounding method that's generally taught in primary school, due to its simplicity (it only ever needs to consider 1 digit).

@parasyte
Copy link

While rounding away from zero is what I learned in elementary school, I don't think that what young children are taught is generally what people (engineers) would expect.

@RalfJung
Copy link
Member

RalfJung commented Jan 11, 2024

While rounding away from zero is what I learned in elementary school, I don't think that what young children are taught is generally what people (engineers) would expect.

In my totally representative survey of 2 CS PhD students, neither of them knew why "round ties to even" would be preferable over "round away from zero". I think it's not just elementary school children that are surprised by the seemingly unnecessarily complicated "ties to even" semantics.

@TennyZhuang
Copy link
Contributor

TennyZhuang commented Jan 11, 2024

Changing the behavior of round is definitely dangerous even across the edition boundaries. Users may focus on the compilation failure, but not the runtime breaking change.

If it’s better to change the function to correct behavior, I propose to fully remove round function in edition 2024, and introduce round_ties_zero instead. This compilation error can be easily fixed automatically.

We can introduce round back in edition 2027, since I believe few crate will upgrade across three editions.

@Amanieu Amanieu removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Jan 16, 2024
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jan 19, 2024
@rfcbot
Copy link

rfcbot commented Jan 19, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@bors bors closed this as completed in 862d3fe Jan 20, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jan 20, 2024
Rollup merge of rust-lang#120150 - Jules-Bertholet:stabilize-round-ties-even, r=cuviper

Stabilize `round_ties_even`

Closes  rust-lang#96710

`@rustbot` label -T-libs T-libs-api
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jan 25, 2024
Chris00 added a commit to Chris00/ITF1788 that referenced this issue Jul 26, 2024
unageek pushed a commit to unageek/ITF1788 that referenced this issue Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-floating-point Area: Floating point numbers and arithmetic C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.