-
Notifications
You must be signed in to change notification settings - Fork 13.8k
Fix atan2 inaccuracy in documentation #146451
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
Conversation
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.
Maybe it would make more sense to rephrase the existing part in terms of >= +0.0
and <= -0.0
instead of adding a new section?
If you take into account the signs of zeros in the function definition, then it will not make mathematical sense. |
I can suggest this: @GrigorenkoPV what do you think? |
a6f181d
to
90307fd
Compare
Yes, this is what I had in mind, more or less. Though, probably with the sign included in the output too. Also, not sure if the |
b4d0c32
to
7236b23
Compare
LGTM, but let's wait for the assigned reviewer |
library/std/src/num/f128.rs
Outdated
/// * `x = 0`, `y = 0`: `0` | ||
/// * `x >= 0`: `arctan(y/x)` -> `[-pi/2, pi/2]` | ||
/// * `y >= 0`: `arctan(y/x) + pi` -> `(pi/2, pi]` | ||
/// * `y < 0`: `arctan(y/x) - pi` -> `(-pi, -pi/2)` | ||
/// * `x >= +0`, `y >= +0`: `[+0, +pi/2]` | ||
/// * `x >= +0`, `y <= -0`: `[-pi/2, -0]` | ||
/// * `x <= -0`, `y >= +0`: `[+pi/2, +pi]` | ||
/// * `x <= -0`, `y <= -0`: `[-pi, -pi/2]` |
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.
I think the mathematical representation is helpful to keep for the intuition. Maybe a table format would work nicely?
/// | `x` | `y` | Equivalent | Range |
/// |---------|---------|---------------|--------------|
/// | `>= +0` | `>= +0` | `arctan(y/x)` | `[+0, +π/2]` |
/// ...
Basically a combination of the piecewise bit from https://en.wikipedia.org/wiki/Atan2#Definition_and_computation with the format of https://en.wikipedia.org/wiki/Inverse_trigonometric_functions#Domains.
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.
While you're here, would you mind also adding a note about how self
and other
map to x
and y
? The (y, x)
argument order can be a bit surprising.
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.
I didn't understand. It's already has note about self (y), other(x).
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.
With your approach, I can suggest this:
/// | `x` | `y` | Equivalent | Range |
/// |---------|---------|-------------------|---------------|
/// | `>= +0` | `>= +0` | `arctan(y/x)` | `[+0, +π/2]` |
/// | `>= +0` | `<= -0` | `arctan(y/x)` | `[-pi/2, -0]` |
/// | `<= -0` | `>= +0` | `arctan(y/x) + pi`| `[+pi/2, +pi]`|
/// | `<= -0` | `<= -0` | `arctan(y/x) - pi`| `[-pi, -pi/2]`|
But from a mathematical point of view, the signed zero here has no meaning, so it looks a bit confusing. And equivalent here is not a true for x, y in this inequalities.
I can change "Equivalent" to "Expression used" or "Definition" so this will more reliable.
What do you think?
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.
I didn't understand. It's already has note about self (y), other(x).
Ah yeah, totally overlooked that.
But from a mathematical point of view, the signed zero here has no meaning, so it looks a bit confusing.
What specifically? The current PR already makes use of +/-0, this seems reasonable to me.
I can change "Equivalent" to "Expression used" or "Definition" so this will more reliable.
"Definition" seems reasonable to me. Or maybe "Piecewise Definition".
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.
I have updated PR, "Piecewise Definition" - really good.
7236b23
to
d834935
Compare
So, what's next? |
Me getting a chance to circle back :) Looks excellent! Thank you for the updates, and @GrigorenkoPV for reviewing this. @bors r+ rollup |
…ocs, r=tgross35 Fix atan2 inaccuracy in documentation Fixes rust-lang#136275
Rollup of 14 pull requests Successful merges: - #142670 (Document fully-qualified syntax in `as`' keyword doc) - #144908 (Fix doctest output json) - #145685 (add CloneFromCell and Cell::get_cloned) - #146330 (Bump unicode_data and printables to version 17.0.0) - #146451 (Fix atan2 inaccuracy in documentation) - #146479 (add mem::conjure_zst) - #147190 (std: `sys::net` cleanups) - #147245 (only replace the intended comma in pattern suggestions) - #147251 (Do not assert that a change in global cache only happens when concurrent) - #147269 (Add regression test for 123953) - #147277 (Extract common logic for iterating over features) - #147280 (Return to needs-llvm-components being info-only) - #147292 (Respect `-Z` unstable options in `rustdoc --test`) - #147300 (Add xtensa arch to object file creation) r? `@ghost` `@rustbot` modify labels: rollup
…ocs, r=tgross35 Fix atan2 inaccuracy in documentation Fixes rust-lang#136275
…ocs, r=tgross35 Fix atan2 inaccuracy in documentation Fixes rust-lang#136275
…ocs, r=tgross35 Fix atan2 inaccuracy in documentation Fixes rust-lang#136275
…ocs, r=tgross35 Fix atan2 inaccuracy in documentation Fixes rust-lang#136275
Rollup of 14 pull requests Successful merges: - #142670 (Document fully-qualified syntax in `as`' keyword doc) - #145685 (add CloneFromCell and Cell::get_cloned) - #146330 (Bump unicode_data and printables to version 17.0.0) - #146451 (Fix atan2 inaccuracy in documentation) - #146479 (add mem::conjure_zst) - #146874 (compiler: Hint at multiple crate versions if trait impl is for wrong ADT ) - #147117 (interpret `#[used]` as `#[used(compiler)]` on illumos) - #147190 (std: `sys::net` cleanups) - #147251 (Do not assert that a change in global cache only happens when concurrent) - #147280 (Return to needs-llvm-components being info-only) - #147288 (compiletest: Make `DirectiveLine` responsible for name/value splitting) - #147309 (Add documentation about unwinding to wasm targets) - #147315 (bless autodiff batching test) - #147323 (Fix top level ui tests check in tidy) r? `@ghost` `@rustbot` modify labels: rollup
…ocs, r=tgross35 Fix atan2 inaccuracy in documentation Fixes rust-lang#136275
Rollup of 11 pull requests Successful merges: - #142670 (Document fully-qualified syntax in `as`' keyword doc) - #145685 (add CloneFromCell and Cell::get_cloned) - #146330 (Bump unicode_data and printables to version 17.0.0) - #146451 (Fix atan2 inaccuracy in documentation) - #146479 (add mem::conjure_zst) - #147117 (interpret `#[used]` as `#[used(compiler)]` on illumos) - #147190 (std: `sys::net` cleanups) - #147251 (Do not assert that a change in global cache only happens when concurrent) - #147280 (Return to needs-llvm-components being info-only) - #147288 (compiletest: Make `DirectiveLine` responsible for name/value splitting) - #147315 (bless autodiff batching test) r? `@ghost` `@rustbot` modify labels: rollup
…ocs, r=tgross35 Fix atan2 inaccuracy in documentation Fixes rust-lang#136275
Rollup of 10 pull requests Successful merges: - #142670 (Document fully-qualified syntax in `as`' keyword doc) - #145685 (add CloneFromCell and Cell::get_cloned) - #146330 (Bump unicode_data and printables to version 17.0.0) - #146451 (Fix atan2 inaccuracy in documentation) - #146479 (add mem::conjure_zst) - #147117 (interpret `#[used]` as `#[used(compiler)]` on illumos) - #147190 (std: `sys::net` cleanups) - #147251 (Do not assert that a change in global cache only happens when concurrent) - #147280 (Return to needs-llvm-components being info-only) - #147315 (bless autodiff batching test) r? `@ghost` `@rustbot` modify labels: rollup
Fixes #136275