-
Notifications
You must be signed in to change notification settings - Fork 13.7k
improve float to_degrees/to_radians rounding comments and impl #145625
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
improve float to_degrees/to_radians rounding comments and impl #145625
Conversation
library/core/src/num/f16.rs
Outdated
pub const fn to_degrees(self) -> Self { | ||
// Use a literal for better precision. | ||
const PIS_IN_180: f16 = 57.2957795130823208767981548141051703_f16; | ||
self * PIS_IN_180 | ||
self * 57.2957795130823208767981548141051703_f16 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The results here will be identical. I meant something like what f64 does where the result is calculated (possibly to a const
),
rust/library/core/src/num/f64.rs
Lines 871 to 876 in 16ad385
pub const fn to_degrees(self) -> f64 { | |
// The division here is correctly rounded with respect to the true | |
// value of 180/π. (This differs from f32, where a constant must be | |
// used to ensure a correctly rounded result.) | |
self * (180.0f64 / consts::PI) | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you're saying we can just merge this? And why the behavior difference for f64?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait, you mean the to_degrees() is a const fn, which in turn could increase precision for constant which uses to_degrees()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite :) I mean that it makes absolutely no difference whether we multiply by the literal 57.2957795130823208767981548141051703, or assign it to a const
first, or let
. They compiler guarantees that the comptime and runtime semantics are the same here.
What I do mean is for f16
, 57.295... is not exactly the same as 180 / pi
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=85a373861e735f8c2a58b00b4540a150. And using the value of 180 / pi
happens to roundtrip for an input of 180.0, though not for other values.
I'm not sure which value (exact at larger precision or computed as f16) should be preferred though, @beetrees is more likely to have this knowledge ready to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exact value of 180 / pi
(57.295...) should be used. This should give more accurate results most of the time as rounding occurs fewer times.
Not directly related to this PR: the to_degrees
and to_radians
functions on floats don't currently document the precision of the result. They should probably have an "Unspecified precision" note similar to most other mathematical functions, as we don't currently provide any guarantees.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we should use the exact value for f32 as well right?
rust/library/core/src/num/f32.rs
Lines 874 to 877 in ed06877
pub const fn to_radians(self) -> f32 { | |
const RADS_PER_DEG: f32 = consts::PI / 180.0; | |
self * RADS_PER_DEG | |
} |
And why is it okay for f64?
rust/library/core/src/num/f64.rs
Lines 871 to 876 in ed06877
pub const fn to_degrees(self) -> f64 { | |
// The division here is correctly rounded with respect to the true | |
// value of 180/π. (This differs from f32, where a constant must be | |
// used to ensure a correctly rounded result.) | |
self * (180.0f64 / consts::PI) | |
} |
Is this simply because f64 has enough precision?
If so then why aren't we doing the same for f128?
rust/library/core/src/num/f128.rs
Lines 647 to 651 in ed06877
pub const fn to_degrees(self) -> Self { | |
// Use a literal for better precision. | |
const PIS_IN_180: f128 = 57.2957795130823208767981548141051703324054724665643215491602_f128; | |
self * PIS_IN_180 | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so I think we should:
- leave the constant literals
- replace the comments
// Use a literal for better precision.
with something along the lines
// Use a literal to avoid division rounding
- add
Unspecified precision
comment toto_degrees
andto_radians
conversions for all floats
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this simply because f64 has enough precision?
If so then why aren't we doing the same for f128?
I've tested it, and 180.0 / consts::PI == 57.2957795130823208767981548141051703
is false
for f16
and f32
but true
for f64
and f128
. Therefore f128::to_degrees
can also just use 180.0 / consts::PI
like f64::to_degrees
does.
with something along the lines
The comment should probably mention that using the literal avoids the double rounding that occurs when using 180.0 / consts::PI
to calculate the constant (the value of consts::PI
is already rounded from the true irrational value of π, with the division then rounding again as you've stated. For f64
and f128
, it just coincidentally happens that the double-rounded value of 180.0 / consts::PI
is the same as the correctly rounded exact value).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f16: false
f32: true
f64: true
f128: false
and it matches with the to_radians implementation, f32 and f64 use division.
ed06877
to
f734a50
Compare
Requested reviewer is already assigned to this pull request. Please choose another assignee. |
I tried to address all of changes we discussed. |
Failed to set assignee to
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
f734a50
to
21a7f52
Compare
update:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @beetrees for reviewing.
One remaining nit @karolzwolak, could you rewrap comments to 100 chars? Both doc and normal comments seem to be ~115.
Please squash as well |
* revise comments explaining why we are using literal or expression * add unspecified precision comments as we don't guarantee precision * use expression in `f128::to_degrees()` * make `f64::to_degrees()` impl consistent with other functions
21a7f52
to
698db13
Compare
Done. @rustbot review |
Looks great, thank you! @bors r=beetrees,tgross35 |
Rollup of 6 pull requests Successful merges: - #144274 (add Option::reduce) - #145562 (Simplify macro generating ToString implementations for `&…&str`) - #145625 (improve float to_degrees/to_radians rounding comments and impl) - #145740 (Introduce a `[workspace.dependencies`] section in the top-level `Cargo.toml`) - #145885 (Inherit TCC in debuginfo tests on macOS) - #145905 (Stop calling unwrap when format foreign has trailing dollar) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #145625 - karolzwolak:f16-use-expr-instead-literal, r=beetrees,tgross35 improve float to_degrees/to_radians rounding comments and impl This PR makes `to_degrees()` and `to_radians()` float functions more consistent between each other and improves comments around their precision and rounding. * revise comments explaining why we are using literal or expression * add unspecified precision comments as we don't guarantee precision * use expression in `f128::to_degrees()` * make `f64::to_degrees()` impl consistent with other functions r? `@tgross35`
This PR makes
to_degrees()
andto_radians()
float functions more consistent between each other and improves comments around their precision and rounding.f128::to_degrees()
f64::to_degrees()
impl consistent with other functionsr? @tgross35