Expose tests for {f32,f64}.total_cmp in docs #115412

eswartz · 2023-08-31T15:56:58Z

Expose tests for {f32,f64}.total_cmp in docs

Uncomment the helpful assert_eq! line, which is stripped out completely in docs, and leaves the reader to mentally play through the algorithm, or go to the playground and add a println!, to see what the result will be.

(If these tests are known to fail on some platforms, is there some mechanism to conditionalize this or escape the test so the assert_eq! source will be visible on the web? I am a newbie, which is why I was reading docs ;)

rustbot · 2023-08-31T15:57:06Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @scottmcm (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

eswartz · 2023-08-31T16:58:54Z

Hmm, I see that #73328 might already explain why this is commented out (i.e. that the sign bit may change on NaN unexpectedly under some optimization cases) but I'm not sure this would occur with the constants used in the sample code.

RalfJung · 2023-09-05T14:17:17Z

It might be due to #115567, in which case adding a suitable cfg that disables the assertion on target_arch = "x86" might help.

RalfJung · 2023-09-05T14:20:01Z

OTOH, # means the test is actually being run, so I guess it does work despite #115567. I see no reason why we'd hide this test from users.

workingjubilee · 2023-10-06T04:45:52Z

I'm approving this under the interpretation that a test passing does not constitute an API guarantee.

@bors r+

bors · 2023-10-06T04:45:54Z

📌 Commit b7d8417 has been approved by workingjubilee

It is now in the queue for this repository.

RalfJung · 2023-10-06T05:34:17Z

I do generally interpret doctests that are shown in rustdoc as lib API guarantees. If I can't rely on the explicitly given examples then what can I rely on?

workingjubilee · 2023-10-06T05:37:37Z

Ah, well,
@bors r-

RalfJung · 2023-10-06T05:39:41Z

That said, is there any reason not to make this an API guarantee?

scottmcm · 2023-10-06T06:33:17Z

Hmm, seems like it'd have to be a lang guarantee, no? Because it's obviously a guarantee in a world where the library code always gets exactly the nans they wrote, and they never change out from under them.

This test needs it to stay the canonical nan, I guess, since it's comparing with to_bits()? If NANs are allowed to spuriously change when passed around or loaded, it's not trivially true to me that that would continue to work...

I guess the test could be updated to check bits-equal for everything except the NAN, and check is_nan for the other? That would be guaranteed, I think. (Especially if it worked by removing the NANs from both ends as the way to find the things that will be exact bits-equal.)

RalfJung · 2023-10-06T09:17:10Z

Don't we have consensus that NaN bits stay stable when floats are just passed around?

Should we do t-lang FCP on that question? This doesn't have to wait for the broader set of questions around what exactly the NaNs are that come out of a float operation.

RalfJung · 2023-10-06T09:19:35Z

Though I guess there also is the question of what we guarantee about the bit pattern in f32::NAN, except that it is a NAN. For instance, do we guarantee the sign? The test will fail if it becomes a negative NAN one day.

The docs about that constant even explicitly say

This constant isn’t guaranteed to equal to any specific NaN bitpattern, and the stability of its representation over Rust versions and target platforms isn’t guaranteed.

So, in that sense the test is already bogus.

eswartz · 2023-10-06T21:17:23Z

Given the discussion, I updated the PR to make it clear there may be different results. (My main concern was just seeing an example with an expected output, since "#" inside the comment -- likely done for the same reasons discussed here -- was stripped out of the HTML.)

workingjubilee · 2023-10-06T21:37:31Z

library/core/src/num/f32.rs

+    /// ```
+    /// Since the NaN constant does not have a well-defined sign,
+    /// this test may result in either:
+    /// ```
+    /// assert!(bois.into_iter().map(|b| b.weight)
+    ///     .zip([-5.0, 0.1, 10.0, 99.0, f32::INFINITY, f32::NAN].iter())
+    ///     .all(|(a, b)| a.to_bits() == b.to_bits()))
+    /// ```
+    /// or:
+    /// ```
+    /// assert!(bois.into_iter().map(|b| b.weight)
+    ///     .zip([f32::NAN, -5.0, 0.1, 10.0, 99.0, f32::INFINITY].iter())
+    ///     .all(|(a, b)| a.to_bits() == b.to_bits()))


Instead of separating this out (and getting error messages when it can't be compiled) why not make this logical disjunction part of the code: simply wrap this in

if f32::NAN.is_sign_negative() { assert!(..) } else { assert!(..) };

Done (for real?)

workingjubilee · 2023-10-06T21:40:36Z

Don't we have consensus that NaN bits stay stable when floats are just passed around?

They should. They should support checking their sign without that changing, too.

RalfJung · 2023-10-07T06:14:59Z

library/core/src/num/f32.rs

-    /// #     .zip([-5.0, 0.1, 10.0, 99.0, f32::INFINITY, f32::NAN].iter())
-    /// #     .all(|(a, b)| a.to_bits() == b.to_bits()))
+    ///
+    /// if f32::NAN.is_sign_negative() {


Suggested change

/// if f32::NAN.is_sign_negative() {

//// // `f32::NAN` could be positive or negative, which will affect the sort order.

/// if f32::NAN.is_sign_negative() {

Applied (manually) since f64.fs also needs the change.

Uncomment the assert! line and account and document that the sign of NaN is not positive, necessarily.

RalfJung

LGTM.

scottmcm · 2023-11-18T05:16:03Z

@bors r+

bors · 2023-11-18T05:16:06Z

📌 Commit 8066079 has been approved by scottmcm

It is now in the queue for this repository.

bors · 2023-11-18T08:49:07Z

⌛ Testing commit 8066079 with merge 6416e2e...

bors · 2023-11-18T10:46:26Z

☀️ Test successful - checks-actions
Approved by: scottmcm
Pushing 6416e2e to master...

bors · 2023-11-18T10:46:26Z

☀️ Test successful - checks-actions
Approved by: scottmcm
Pushing 6416e2e to master...

rust-timer · 2023-11-18T12:07:26Z

Finished benchmarking commit (6416e2e): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.6%, 0.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.3%	[-2.3%, -2.3%]	1
All ❌✅ (primary)	0.6%	[0.6%, 0.6%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.2%	[3.2%, 3.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-3.1%, -1.9%]	6
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 678.192s -> 675.683s (-0.37%)
Artifact size: 313.84 MiB -> 313.83 MiB (-0.00%)

rustbot assigned scottmcm Aug 31, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 31, 2023

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 Oct 6, 2023

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 6, 2023

eswartz force-pushed the docs/total_cmp-test-result-in-docs branch from b7d8417 to 08f10c2 Compare October 6, 2023 21:13

This comment has been minimized.

Sign in to view

workingjubilee reviewed Oct 6, 2023

View reviewed changes

workingjubilee added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 6, 2023

eswartz force-pushed the docs/total_cmp-test-result-in-docs branch 2 times, most recently from cfc285d to 2a62677 Compare October 6, 2023 22:50

RalfJung reviewed Oct 7, 2023

View reviewed changes

Expose tests for {f32,f64}.total_cmp in docs, along with comment

8066079

Uncomment the assert! line and account and document that the sign of NaN is not positive, necessarily.

eswartz force-pushed the docs/total_cmp-test-result-in-docs branch from 2a62677 to 8066079 Compare October 7, 2023 16:19

RalfJung approved these changes Oct 7, 2023

View reviewed changes

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 Nov 18, 2023

bors added merged-by-bors This PR was explicitly merged by bors. labels Nov 18, 2023

bors merged commit 6416e2e into rust-lang:master Nov 18, 2023
12 checks passed

rustbot added this to the 1.76.0 milestone Nov 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose tests for {f32,f64}.total_cmp in docs #115412

Expose tests for {f32,f64}.total_cmp in docs #115412

eswartz commented Aug 31, 2023

rustbot commented Aug 31, 2023

eswartz commented Aug 31, 2023

RalfJung commented Sep 5, 2023 •

edited

Loading

RalfJung commented Sep 5, 2023

workingjubilee commented Oct 6, 2023

bors commented Oct 6, 2023

RalfJung commented Oct 6, 2023 •

edited

Loading

workingjubilee commented Oct 6, 2023

RalfJung commented Oct 6, 2023

scottmcm commented Oct 6, 2023

RalfJung commented Oct 6, 2023

RalfJung commented Oct 6, 2023

eswartz commented Oct 6, 2023

This comment has been minimized.

workingjubilee Oct 6, 2023 •

edited

Loading

eswartz Oct 6, 2023 •

edited

Loading

workingjubilee commented Oct 6, 2023

RalfJung Oct 7, 2023

eswartz Oct 7, 2023

RalfJung left a comment

scottmcm commented Nov 18, 2023

bors commented Nov 18, 2023

bors commented Nov 18, 2023

bors commented Nov 18, 2023

bors commented Nov 18, 2023

rust-timer commented Nov 18, 2023

	/// if f32::NAN.is_sign_negative() {
	//// // `f32::NAN` could be positive or negative, which will affect the sort order.
	/// if f32::NAN.is_sign_negative() {

Expose tests for {f32,f64}.total_cmp in docs #115412

Expose tests for {f32,f64}.total_cmp in docs #115412

Conversation

eswartz commented Aug 31, 2023

rustbot commented Aug 31, 2023

eswartz commented Aug 31, 2023

RalfJung commented Sep 5, 2023 • edited Loading

RalfJung commented Sep 5, 2023

workingjubilee commented Oct 6, 2023

bors commented Oct 6, 2023

RalfJung commented Oct 6, 2023 • edited Loading

workingjubilee commented Oct 6, 2023

RalfJung commented Oct 6, 2023

scottmcm commented Oct 6, 2023

RalfJung commented Oct 6, 2023

RalfJung commented Oct 6, 2023

eswartz commented Oct 6, 2023

This comment has been minimized.

workingjubilee Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

eswartz Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

workingjubilee commented Oct 6, 2023

RalfJung Oct 7, 2023

Choose a reason for hiding this comment

eswartz Oct 7, 2023

Choose a reason for hiding this comment

RalfJung left a comment

Choose a reason for hiding this comment

scottmcm commented Nov 18, 2023

bors commented Nov 18, 2023

bors commented Nov 18, 2023

bors commented Nov 18, 2023

bors commented Nov 18, 2023

rust-timer commented Nov 18, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

RalfJung commented Sep 5, 2023 •

edited

Loading

RalfJung commented Oct 6, 2023 •

edited

Loading

workingjubilee Oct 6, 2023 •

edited

Loading

eswartz Oct 6, 2023 •

edited

Loading