Fix atan2 inaccuracy in documentation

[DimitriiTrater]

Fixes #136275

[rustbot]

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[GrigorenkoPV]

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?

View changes since this review

[DimitriiTrater]

If you take into account the signs of zeros in the function definition, then it will not make mathematical sense.
Signs of zeros in this context make sense only if we are talking about IEEE-754.
If you do it the way you say, then I need to remove the mathematical definition of the function from arctan and leave only the areas of incoming and outgoing values.

[DimitriiTrater]

I can suggest this:
x >= +0, y >= +0: [0, π/2]
x <= -0, y >= +0: [π/2, π]
x >= +0, y <= -0: [-π/2, 0]
x <= -0, y <= -0: [-π, -π/2]

@GrigorenkoPV what do you think?

[GrigorenkoPV]

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 pi should be changed to π here.

[GrigorenkoPV]

LGTM, but let's wait for the assigned reviewer

[tgross35]

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.

[tgross35]

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.

[DimitriiTrater]

I didn't understand. It's already has note about self (y), other(x).

[DimitriiTrater]

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?

[tgross35]

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".

[DimitriiTrater]

I have updated PR, "Piecewise Definition" - really good.

[DimitriiTrater]

So, what's next?

[tgross35]

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

[bors]

📌 Commit d834935 has been approved by tgross35

It is now in the queue for this repository.