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

Improve floating point documentation #95483

Merged
merged 8 commits into from
May 9, 2022

Conversation

golddranks
Copy link
Contributor

@golddranks golddranks commented Mar 30, 2022

This is my attempt to improve/solve #95468 and #73328 .

Added/refined explanations:

  • Refine the "NaN as a special value" top level explanation of f32
  • Refine const NAN docstring: add an explanation about there being multitude of NaN bitpatterns and disclaimer about the portability/stability guarantees.
  • Refine fn is_sign_positive and fn is_sign_negative docstrings: add disclaimer about the sign bit of NaNs.
  • Refine fn min and fn max docstrings: explain the semantics and their relationship to the standard and libm better.
  • Refine fn trunc docstrings: explain the semantics slightly more.
  • Refine fn powi docstrings: add disclaimer that the rounding behaviour might be different from powf.
  • Refine fn copysign docstrings: add disclaimer about payloads of NaNs.
  • Refine minimum and maximum: add disclaimer that "propagating NaN" doesn't mean that propagating the NaN bit patterns is guaranteed.
  • Refine max and min docstrings: add "ignoring NaN" to bring the one-row explanation to parity with minimum and maximum.

Cosmetic changes:

  • Reword NaN and NAN as plain "NaN", unless they refer to the specific const NAN.
  • Reword "a number" to self in function docstrings to clarify.
  • Remove "Returns NAN if the number is NAN" from abs, as this is told to be the default behavior in the top explanation.

- Refine the "NaN as a special value" top level explanation of f32
- Refine `const NAN` docstring.
- Refine `fn is_sign_positive` and `fn is_sign_negative` docstrings.
- Refine `fn min` and `fn max` docstrings.
- Refine `fn trunc` docstrings.
- Refine `fn powi` docstrings.
- Refine `fn copysign` docstrings.
- Reword `NaN` and `NAN` as plain "NaN", unless they refer to the specific `const NAN`.
- Reword "a number" to `self` in function docstrings to clarify.
- Remove "Returns NAN if the number is NAN" as this is told to be the default behavior in the top explanation.
- Remove "propagating NaNs", as full propagation (preservation of payloads) is not guaranteed.
@rust-highfive
Copy link
Collaborator

r? @yaahc

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 30, 2022
@golddranks golddranks changed the title Improve floating point documentation: Improve floating point documentation Mar 30, 2022
@golddranks
Copy link
Contributor Author

golddranks commented Mar 30, 2022

Also attempts to fix (documentation issues of) #71355

@golddranks
Copy link
Contributor Author

I think that people who were active over #72599 might have something to say about the wording here...?

@tbu-
Copy link
Contributor

tbu- commented Mar 31, 2022

I think it might be a good idea to keep the original wording of

  • Remove "Returns NAN if the number is NAN" as this is told to be the default behavior in the top explanation.

as you'd otherwise require the reader to read the whole documentation instead of just the one on the function they're interested in.

@golddranks
Copy link
Contributor Author

@tbu- I see. In that case, I think we should add it to other operations too. That's entirely doable, but bloats the explanations a bit.

@tbu-
Copy link
Contributor

tbu- commented Mar 31, 2022

It's especially interesting for the min/max functions because we provide two functions with different semantics. I liked that you moved the explanation about NaNs to the start there. :)

In that case, I think we should add it to other operations too.

Which other functions were you thinking about? For stuff like sin, trunc, etc., it seems to me that there's not much the function could do except mapping NaNs to NaNs.

@golddranks
Copy link
Contributor Author

golddranks commented Mar 31, 2022

Well, I only removed the "Returns NAN if the number is NAN" explanation from abs, and I think that's another case where it should be pretty clear that a NaN yields Nan, so stating it explicitly felt like standing out of the line. Yes, min and max are cases which definitely need more explaning, so they'd better have a proper explanation about NaN semantics in the docstrings.

@tbu-
Copy link
Contributor

tbu- commented Mar 31, 2022

Ah, sorry, I pointed out the wrong thing then. Thanks for your patience with me. :) abs not saying anything about that is fine for me.

You removed the NaN propagation from the first line of the minimum/maximum functions. This is the only thing shown in search results: https://doc.rust-lang.org/std/?search=mini. I think it would be good to include something to differentiate between minimum from min in the first line of either function.

@golddranks
Copy link
Contributor Author

golddranks commented Mar 31, 2022

Ahh, I see the problem now: the one-line explanation could be better indeed. I thought the explanation of the semantics after the first line was pretty thorough, so mentioning "propagating NaNs" on the first line wouldn't be needed. The reason I decided to remove it is because I thought the word "propagating" would falsely give the impression that the "propagation/conservation" of NaN bitpatterns would be guaranteed, while making a point not to guarantee that was an important motivation for this PR. (As you can see from the refined "NaN as a special value" top level explanation).

…NaN" to `max`/`min`, add disclaimer about the "propagation".
@golddranks
Copy link
Contributor Author

@tbu- How about this?

@rust-log-analyzer

This comment has been minimized.

@tbu-
Copy link
Contributor

tbu- commented Mar 31, 2022

Yes, this is a lot better, thanks for following up on my comments. :)

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 24, 2022
@yaahc yaahc added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 29, 2022
@yaahc
Copy link
Member

yaahc commented Apr 29, 2022

I'm reassigning this PR because I'm taking a break from the review rotation for a little while. Thank you for your patience.

r? rust-lang/libs

@rust-highfive rust-highfive assigned joshtriplett and unassigned yaahc Apr 29, 2022
@golddranks
Copy link
Contributor Author

golddranks commented Apr 30, 2022

@yaahc It's totally OK!

Btw. it just crossed my mind that we should maybe document things around denormals better too. This PR is somewhat focused to NaN, but I think subnormals have similar problems as NaN patterns: apparently some hardware don't allow for gradual loss of precision, but I'm totally unaware which pieces of hardware, and whether those are currently supported in any capacity by Rust. Also, I wonder what are LLVM's commitments to protect deterministicness of calculations with subnormals.

library/std/src/f32.rs Outdated Show resolved Hide resolved
library/std/src/f64.rs Outdated Show resolved Hide resolved
@joshtriplett
Copy link
Member

TIL that we have both min/max and minimum/maximum, and they have different semantics with respect to NaNs.

This documentation seems like an improvement across the board, thank you! Posted suggestions with minor grammatical nits. r=me with those applied.

@joshtriplett
Copy link
Member

@bors d+

@golddranks
Copy link
Contributor Author

@bors r=joshtriplett

@bors
Copy link
Contributor

bors commented May 2, 2022

@golddranks: 🔑 Insufficient privileges: Not in reviewers

@golddranks
Copy link
Contributor Author

@joshtriplett Sorry, it seems that bors didn't accept the delegation command. Could you accept it?

@davidtwco

This comment was marked as outdated.

@davidtwco

This comment was marked as outdated.

@davidtwco
Copy link
Member

@bors r=joshtriplett

@bors
Copy link
Contributor

bors commented May 9, 2022

📌 Commit dea7765 has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 9, 2022
@golddranks
Copy link
Contributor Author

@davidtwco Thank you!

@davidtwco
Copy link
Member

@bors delegate+

@bors
Copy link
Contributor

bors commented May 9, 2022

✌️ @golddranks can now approve this pull request

@scottmcm
Copy link
Member

scottmcm commented May 9, 2022

TIL that we have both min/max and minimum/maximum

Yeah, we have both, but that's because IEEE also has both -- though IIRC it has deprecated min/max for having strange NAN handling, as they work differently from how basically every other float API handles NANs.

@golddranks
Copy link
Contributor Author

Yep, the standard min/max works differently for sNaNs and qNaNs, which leads to strange cases where depending how you group the operations you get either the the non-NaN operand or a NaN as the result. We (and libm) at least have "sane" although non-standard semantics. And I think it's good to have both this non-standard but sane min/max and the newly standardized minimum/maximum, as ignoring and propagating NaNs are both valid usecases.

@golddranks
Copy link
Contributor Author

@bors rollup=always

bors added a commit to rust-lang-ci/rust that referenced this pull request May 9, 2022
…askrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#95483 (Improve floating point documentation)
 - rust-lang#96008 (Warn on unused `#[doc(hidden)]` attributes on trait impl items)
 - rust-lang#96841 (Revert "Implement [OsStr]::join", which was merged without FCP.)
 - rust-lang#96844 (Actually fix ICE from rust-lang#96583)
 - rust-lang#96854 (Some subst cleanup)
 - rust-lang#96858 (Remove unused param from search.js::checkPath)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 28d800c into rust-lang:master May 9, 2022
@rustbot rustbot added this to the 1.62.0 milestone May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.