Document round-off error in .mod_euc()-method, see issue #50179
#50342
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
clamydo
force-pushed
the
euclidean
branch
2 times, most recently
from
47eb521
to
053b564
Compare
scottmcm
reviewed
May 1, 2018
| @@ -254,6 +257,8 @@ impl f32 { | |||
| /// assert_eq!((-a).mod_euc(b), 1.0); | |||
| /// assert_eq!(a.mod_euc(-b), 3.0); | |||
| /// assert_eq!((-a).mod_euc(-b), 1.0); | |||
| /// // limitation due to round-off error | |||
| /// assert!((-std::f32::EPSILON).mod_euc(3.0) != 0.0); | |||
There was a problem hiding this comment.
This has reliable output, right? assuming so, it'd be nice to phrase it positively instead:
assert_eq!((-std::f32::EPSILON).mod_euc(3.0), 3.0);Also, while you're here, maybe you could talk about how this treats NaNs and Infinities? For example, I see that currently,
assert!((std::f32::INFINITY).mod_euc(3.0).is_nan());(Did the RFC talk about what's supposed to happen in such cases?)
There was a problem hiding this comment.
Floating point arithmetic is many things, but it is deterministic 😉. I could turn this assertion around, but I put it that way ('not equal to') to stress, that it results in something one might not expect. The way proposed by you reads more like expected behaviour.
The RFC did not talk about border cases of floats. I could also add these, if you'd like that:
assert!(std::f64::INFINITY.mod_euc(a).is_nan());
assert_eq!(a.mod_euc(std::f64::INFINITY), a);
assert!(a.mod_euc(std::f64::NAN).is_nan());
assert!(std::f64::INFINITY.mod_euc(std::f64::INFINITY).is_nan());
assert!(std::f64::INFINITY.mod_euc(std::f64::NAN).is_nan());
assert!(std::f64::NAN.mod_euc(std::f64::INFINITY).is_nan());which all are true as expected. In particular assert_eq!(a.mod_euc(std::f64::INFINITY), a); is nice. The rest might be a bit noisy?
There was a problem hiding this comment.
The way proposed by you reads more like expected behaviour.
I don't know what's best. I kind-of like not calling it error, but putting a positive spin on it and saying "yes, this is the closest float to the mathematically-correct value".
Whatever you choose to do, as someone from the "but I thought the whole point was to have a half-open range" camp, I'd like to see enough of the "why this behaviour isn't a bug" discussion in the docs to keep someone from me from opening another issue about it 😆
The rest might be a bit noisy?
Maybe pick some that are particularly interesting for the examples section, describe the usual things like "any input being NaN gives NaN" in prose, and add (non-doc-)tests for the rest? The only such tests I could find for mod_euc so far is this:
rust/src/libcore/tests/num/int_macros.rs
Lines 33 to 36 in 52ed3d8
|
Ping from triage, @withoutboats ! This is awaiting your review. |
pietroalbini
added
the
T-libs-api
|
Ping from triage! This PR needs a review, can @withoutboats or someone else from @rust-lang/libs review this? |
|
someone else from libs please, I don't feel confident to review the accuracy of these comments :) |
pietroalbini
added
S-waiting-on-author
|
I'm going to add the unit tests for the special float values ( But I find it hard to include a rational on choosing the given drawbacks in the documentation, because I'm not fully convinced, this is the way to go for this method. The reason I haven't done the above already is that ideally I'd like to proof that |
|
Ping from triage, @fkjogu: could you give us an update on your plans for this PR? |
|
I've added a remark about the rationale of the implementation, although I haven't proven the property. I just ran some tests on edge cases I came up with. I've also added some unit tests. Currently this is true but I'd argue, that 'NAN' would be more correct. It is not completely off, since yes, a number fits into infinity infinity times. But ultimately it is just not defined. From my point of view, this is good to go. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Looks like there may be some travis errors to help clean up? |
|
I blindly added the tests to |
|
Ah anywhere within |
|
I've moved the unit tests to and I do not understand why. Sorry for the trouble. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
A floating point literal like The difference is that the first test uses a known-type value (e.g. INFINITY) first. Since there's only the one implementation of the method when the receiver type is known, type inference can pin down the type of #![feature(euclidean_division)]
use std::f32::INFINITY;
#[test]
fn mod_euc() {
let a = 42.0;
assert!(INFINITY.mod_euc(a).is_nan());
}
#[test]
fn div_euc() {
let a = 42.0;
assert_eq!(a.div_euc(INFINITY), 0.0);
}TL;DR: pin down the type: let a: $fty = 42.0; |
|
Thanks, that solved the issue. Good to go. |
pietroalbini
added
S-waiting-on-review
|
The reviewer is away, maybe someone else from @rust-lang/libs can review this? |
1 similar comment
|
The reviewer is away, maybe someone else from @rust-lang/libs can review this? |
|
r? @BurntSushi |
|
The diff LGTM and it looks like other folks who know more about this topic than I do are happy as well, so @bors r+ (@Mark-Simulacrum FYI, I removed myself from the rust-highfive rotation because my time is so constrained. Happy to do the occasional review though!) |
|
📌 Commit b5847bf37c2235fd10af46009a614423de214e39 has been approved by |
bors
added
S-waiting-on-bors
|
🔒 Merge conflict This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again. How do I rebase?Assuming
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Error message |
bors
added
S-waiting-on-author
|
@fkjogu you need to rebase on the latest master, even if you don't see any conflict (sorry about that!) |
|
I've rebased (a few hours ago). How can I check, if I need to rebase again? |
|
That should be enough, thanks! @bors r=BurntSushi rollup |
|
📌 Commit bd853a6 has been approved by |
bors
added
S-waiting-on-bors
kennytm
added a commit
to kennytm/rust
that referenced
this pull request
Jun 27, 2018
Document round-off error in `.mod_euc()`-method, see issue rust-lang#50179 Due to a round-off error the method `.mod_euc()` of both `f32` and `f64` can produce mathematical invalid outputs. If `self` in magnitude is much small than the modulus `rhs` and negative, `self + rhs` in the first branch cannot be represented in the given precision and results into `rhs`. In the mathematical strict sense, this breaks the definition. But given the limitation of floating point arithmetic it can be thought of the closest representable value to the true result, although it is not strictly in the domain `[0.0, rhs)` of the function. It is rather the left side asymptotical limit. It would be desirable that it produces the mathematical more sound approximation of `0.0`, the right side asymptotical limit. But this breaks the property, that `self == self.div_euc(rhs) * rhs + a.mod_euc(rhs)`. The discussion in issue rust-lang#50179 did not find an satisfying conclusion to which property is deemed more important. But at least we can document the behaviour. Which this pull request does.
bors
added a commit
that referenced
this pull request
Jun 27, 2018
Rollup of 7 pull requests Successful merges: - #49987 (Add str::split_ascii_whitespace.) - #50342 (Document round-off error in `.mod_euc()`-method, see issue #50179) - #51658 (Only do sanity check with debug assertions on) - #51799 (Lower case some feature gate error messages) - #51800 (Add a compiletest header for edition) - #51824 (Fix the error reference for LocalKey::try_with) - #51842 (Document that Layout::from_size_align does not allow align=0) Failed merges: r? @ghost
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Due to a round-off error the method
.mod_euc()of bothf32andf64can produce mathematical invalid outputs. Ifselfin magnitude is much small than the modulusrhsand negative,self + rhsin the first branch cannot be represented in the given precision and results intorhs. In the mathematical strict sense, this breaks the definition. But given the limitation of floating point arithmetic it can be thought of the closest representable value to the true result, although it is not strictly in the domain[0.0, rhs)of the function. It is rather the left side asymptotical limit. It would be desirable that it produces the mathematical more sound approximation of0.0, the right side asymptotical limit. But this breaks the property, thatself == self.div_euc(rhs) * rhs + a.mod_euc(rhs).The discussion in issue #50179 did not find an satisfying conclusion to which property is deemed more important. But at least we can document the behaviour. Which this pull request does.