Skip to content

Document round-off error in .mod_euc()-method, see issue #50179 #50342

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jun 28, 2018
Merged

Document round-off error in .mod_euc()-method, see issue #50179 #50342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
af6f0f2
Document round-off error in `.mod_euc()`-method, see issue #50179
fkjogu Apr 30, 2018
daeadc6
Clarify rational behind current implementation of `.mod_euc()`
fkjogu May 28, 2018
bd853a6
Add unit tests for `.mod_euc()` and `.div_euc()`
fkjogu May 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/libcore/tests/num/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,6 +574,25 @@ macro_rules! test_float {
assert_eq!((-9.0 as $fty).max($nan), -9.0);
assert!(($nan as $fty).max($nan).is_nan());
}
#[test]
fn mod_euc() {
let a: $fty = 42.0;
assert!($inf.mod_euc(a).is_nan());
assert_eq!(a.mod_euc($inf), a);
assert!(a.mod_euc($nan).is_nan());
assert!($inf.mod_euc($inf).is_nan());
assert!($inf.mod_euc($nan).is_nan());
assert!($nan.mod_euc($inf).is_nan());
}
#[test]
fn div_euc() {
let a: $fty = 42.0;
assert_eq!(a.div_euc($inf), 0.0);
assert!(a.div_euc($nan).is_nan());
assert!($inf.div_euc($inf).is_nan());
assert!($inf.div_euc($nan).is_nan());
assert!($nan.div_euc($inf).is_nan());
}
} }
}

Expand Down
11 changes: 10 additions & 1 deletion src/libstd/f32.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,14 @@ impl f32 {

/// Calculates the Euclidean modulo (self mod rhs), which is never negative.
///
/// In particular, the result `n` satisfies `0 <= n < rhs.abs()`.
/// In particular, the return value `r` satisfies `0.0 <= r < rhs.abs()` in
/// most cases. However, due to a floating point round-off error it can
/// result in `r == rhs.abs()`, violating the mathematical definition, if
/// `self` is much smaller than `rhs.abs()` in magnitude and `self < 0.0`.
/// This result is not an element of the function's codomain, but it is the
/// closest floating point number in the real numbers and thus fulfills the
/// property `self == self.div_euc(rhs) * rhs + self.mod_euc(rhs)`
/// approximatively.
///
/// # Examples
///
Expand All @@ -266,6 +273,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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#[test]
fn test_mod_euc() {
assert!((-1 as $T).mod_euc(MIN) == MAX);
}

/// ```
#[inline]
#[unstable(feature = "euclidean_division", issue = "49048")]
Expand Down
11 changes: 10 additions & 1 deletion src/libstd/f64.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,14 @@ impl f64 {

/// Calculates the Euclidean modulo (self mod rhs), which is never negative.
///
/// In particular, the result `n` satisfies `0 <= n < rhs.abs()`.
/// In particular, the return value `r` satisfies `0.0 <= r < rhs.abs()` in
/// most cases. However, due to a floating point round-off error it can
/// result in `r == rhs.abs()`, violating the mathematical definition, if
/// `self` is much smaller than `rhs.abs()` in magnitude and `self < 0.0`.
/// This result is not an element of the function's codomain, but it is the
/// closest floating point number in the real numbers and thus fulfills the
/// property `self == self.div_euc(rhs) * rhs + self.mod_euc(rhs)`
/// approximatively.
///
/// # Examples
///
Expand All @@ -242,6 +249,8 @@ impl f64 {
/// 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::f64::EPSILON).mod_euc(3.0) != 0.0);
/// ```
#[inline]
#[unstable(feature = "euclidean_division", issue = "49048")]
Expand Down