Improve floating point documentation #95483

golddranks · 2022-03-30T17:18:26Z

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 · 2022-03-30T17:18:29Z

r? @yaahc

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

golddranks · 2022-03-30T17:23:16Z

Also attempts to fix (documentation issues of) #71355

…}_{be,le,ne}_bytes

golddranks · 2022-03-30T17:32:10Z

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

tbu- · 2022-03-31T03:25:18Z

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 · 2022-03-31T03:57:40Z

@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- · 2022-03-31T05:40:01Z

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 · 2022-03-31T06:01:00Z

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- · 2022-03-31T06:10:22Z

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 · 2022-03-31T08:46:11Z

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 · 2022-03-31T09:00:14Z

@tbu- How about this?

tbu- · 2022-03-31T14:52:03Z

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

yaahc · 2022-04-29T22:52:05Z

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

golddranks · 2022-04-30T03:51:18Z

@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/core/src/num/f32.rs

library/std/src/f32.rs

library/std/src/f64.rs

joshtriplett · 2022-05-01T05:48:50Z

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 · 2022-05-01T05:49:00Z

@bors d+

golddranks · 2022-05-02T15:09:23Z

@bors r=joshtriplett

bors · 2022-05-02T15:09:24Z

@golddranks: 🔑 Insufficient privileges: Not in reviewers

golddranks · 2022-05-02T15:11:14Z

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

davidtwco · 2022-05-09T05:24:30Z

@bors r=joshtriplett

bors · 2022-05-09T05:24:31Z

📌 Commit dea7765 has been approved by joshtriplett

golddranks · 2022-05-09T05:25:10Z

@davidtwco Thank you!

davidtwco · 2022-05-09T05:25:34Z

@bors delegate+

bors · 2022-05-09T05:25:36Z

✌️ @golddranks can now approve this pull request

scottmcm · 2022-05-09T05:27:00Z

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 · 2022-05-09T05:31:48Z

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 · 2022-05-09T06:33:58Z

@bors rollup=always

…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

rust-highfive assigned yaahc Mar 30, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 30, 2022

golddranks changed the title ~~Improve floating point documentation:~~ Improve floating point documentation Mar 30, 2022

Fix: is_sign_positive -> is_sign_negative

a965778

Add references to explanation about portability to f{32,64}::{from,to…

fff5a06

…}_{be,le,ne}_bytes

Improve wording of "NaN as a special value" top level explanation

4ee8b64

Re-introduce "propagating NaN" to maximum/minimum, add "ignoring …

57eec0c

…NaN" to `max`/`min`, add disclaimer about the "propagation".

Further refine the disclaimer about NaN bit patterns.

21f1037

This comment has been minimized.

Sign in to view

match std f32 primitive docs to core f32 primitive docs

7175c49

golddranks mentioned this pull request Apr 4, 2022

Tracking Issue for total_cmp (on f32/f64) #72599

Closed

6 tasks

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 added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 29, 2022

rust-highfive assigned joshtriplett and unassigned yaahc Apr 29, 2022

joshtriplett reviewed May 1, 2022

View reviewed changes

library/core/src/num/f32.rs Outdated Show resolved Hide resolved

joshtriplett reviewed May 1, 2022

View reviewed changes

library/core/src/num/f32.rs Outdated Show resolved Hide resolved

joshtriplett reviewed May 1, 2022

View reviewed changes

library/std/src/f32.rs Outdated Show resolved Hide resolved

joshtriplett reviewed May 1, 2022

View reviewed changes

library/std/src/f64.rs Outdated Show resolved Hide resolved

Fix nits

dea7765

This comment was marked as outdated.

Sign in to view

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

matthiaskrgr mentioned this pull request May 9, 2022

Rollup of 6 pull requests #96877

Merged

bors merged commit 28d800c into rust-lang:master May 9, 2022

rustbot added this to the 1.62.0 milestone May 9, 2022

RalfJung mentioned this pull request Jan 6, 2024

Improve floating point documentation about platform guarantees #95468

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve floating point documentation #95483

Improve floating point documentation #95483

golddranks commented Mar 30, 2022 •

edited

rust-highfive commented Mar 30, 2022

golddranks commented Mar 30, 2022 •

edited

golddranks commented Mar 30, 2022

tbu- commented Mar 31, 2022

golddranks commented Mar 31, 2022

tbu- commented Mar 31, 2022

golddranks commented Mar 31, 2022 •

edited

tbu- commented Mar 31, 2022

golddranks commented Mar 31, 2022 •

edited

golddranks commented Mar 31, 2022

This comment has been minimized.

tbu- commented Mar 31, 2022

yaahc commented Apr 29, 2022

golddranks commented Apr 30, 2022 •

edited

joshtriplett commented May 1, 2022

joshtriplett commented May 1, 2022

golddranks commented May 2, 2022

bors commented May 2, 2022

golddranks commented May 2, 2022

This comment was marked as outdated.

This comment was marked as outdated.

davidtwco commented May 9, 2022

bors commented May 9, 2022

golddranks commented May 9, 2022

davidtwco commented May 9, 2022

bors commented May 9, 2022

scottmcm commented May 9, 2022

golddranks commented May 9, 2022

golddranks commented May 9, 2022

Improve floating point documentation #95483

Improve floating point documentation #95483

Conversation

golddranks commented Mar 30, 2022 • edited

rust-highfive commented Mar 30, 2022

golddranks commented Mar 30, 2022 • edited

golddranks commented Mar 30, 2022

tbu- commented Mar 31, 2022

golddranks commented Mar 31, 2022

tbu- commented Mar 31, 2022

golddranks commented Mar 31, 2022 • edited

tbu- commented Mar 31, 2022

golddranks commented Mar 31, 2022 • edited

golddranks commented Mar 31, 2022

This comment has been minimized.

tbu- commented Mar 31, 2022

yaahc commented Apr 29, 2022

golddranks commented Apr 30, 2022 • edited

joshtriplett commented May 1, 2022

joshtriplett commented May 1, 2022

golddranks commented May 2, 2022

bors commented May 2, 2022

golddranks commented May 2, 2022

This comment was marked as outdated.

This comment was marked as outdated.

davidtwco commented May 9, 2022

bors commented May 9, 2022

golddranks commented May 9, 2022

davidtwco commented May 9, 2022

bors commented May 9, 2022

scottmcm commented May 9, 2022

golddranks commented May 9, 2022

golddranks commented May 9, 2022

golddranks commented Mar 30, 2022 •

edited

golddranks commented Mar 30, 2022 •

edited

golddranks commented Mar 31, 2022 •

edited

golddranks commented Mar 31, 2022 •

edited

golddranks commented Apr 30, 2022 •

edited