Fix `sum()` accuracy #10927

g3xzh · 2013-12-12T00:29:08Z

[1e20, 1.0, -1e20].sum() returns 0.0. This happens because during
the summation, 1.0 is too small relative to 1e20, making it
negligible.

I have tried Kahan summation but it hasn't fixed the problem.
Therefore, I've used Python's fsum() implementation.
For more details, read:
www.cs.cmu.edu/~quake-papers/robust-arithmetic.ps
#10851

Python's fsum (msum)
http://code.activestate.com/recipes/393090/

@huonw, your feedback is more than welcome.
It looks unpolished; Do you have suggestions how to make it more beautiful and elegant?

Thanks in advance,

huonw · 2013-12-12T00:35:55Z

src/libextra/stats.rs

+            for &(i1, f1) in modif_tuple.iter() {
+                partials[i1] = f1;
+            }
+            if (partials.len() > i as uint) {


These parens are not necessary.

g3xzh · 2013-12-12T03:05:11Z

I am not sure if the benchmark tests are good enough.

huonw · 2013-12-12T12:13:05Z

src/libextra/stats.rs

-    fn sum(self) -> f64 {
-        self.iter().fold(0.0, |p,q| p + *q)
+    /*
+     * Note: this method sacrifices performance at the altar of accuracy


This should be a doc comment on the method definition in the trait, otherwise, unfortunately, it's not visible in the docs. That is, something like,

/// The sum of the samples. /// /// Note: this method sacrifices performance at the altar of /// accuracy. Depends... [etc.] fn sum(self) -> f64;

(Also, we might as well cite this properly to help guard against the links going dead (i.e. still make it possible to find the paper), e.g. like I have been doing in std::rand, so something like Shewchuk J.R., ["Adaptive Precision Floating-Point Arithmetic and Fast Robust Geometric Predicates"](http://www.cs.cmu.edu/~quake-papers/robust-arithmetic.ps) *Discrete & Computational Geometry 18*, 3 (Oct 1997), 305-363. (the page I'm basing this off, feel free to use some other citation format.))

huonw · 2013-12-12T13:53:57Z

Looks pretty good; address my comments, and rebase on top of master and we should be good to go!

jfager · 2013-12-14T04:22:27Z

For reference, here is how this got added to Python: http://bugs.python.org/issue2819. Their final C implementation handles subtleties with overflow and rounding that direct translation of Raymond Hettinger's python code misses.

huonw · 2013-12-14T04:39:17Z

Ah, thanks for that @jfager; it looks like this python code is referenced from the current code in Python, and is presumably what the C implementation was adapted from.

jfager · 2013-12-14T05:11:05Z

This algo happens to be one that I implement when I'm picking up a new language, so I've got a Rust version that doesn't need the modifications vec. I've put up a comparison of the two approaches at https://gist.github.com/jfager/7955901; it's about 2x as fast w/o the extra vec.

Still needs the overflow and rounding fixes, though.

huonw · 2013-12-14T05:24:19Z

I believe the implementation you took from this PR is actually incorrect, and better comparison would be with the change I suggested here. (e: performing that change still shows a similar performance gap. e2: for larger cases, the constant overhead of the second vector is reduced to make the performance gap about 1.5× (which is still pretty significant), also, using unsafe_set and unsafe_get instead of the checked indexing gives no appreciable difference.)

jfager · 2013-12-14T05:50:56Z

Updated the gist w/ your suggestion, check it (or feel free to fork it) to make sure I'm understanding right. It's still quite a bit slower than w/o modifications.

huonw · 2013-12-15T06:30:36Z

src/libextra/stats.rs

@@ -146,9 +147,45 @@ impl Summary {
 }

 impl<'self> Stats for &'self [f64] {
-
+    /// Note: this method sacrifices performance at the altar of accuracy


This comment needs to add to the declaration of this method in the trait, i.e. there is a line above that says fn sum(self) -> f64; with a short comment above it. This should become:

/// <old comment> /// /// <this comment> fn sum(self) -> f64;

so that this text is the second paragraph of the "main" doc-comment.

g3xzh · 2013-12-18T22:28:47Z

OK, fixed indentation as @adridu59 has mentioned, and ran "make check".

@huonw, waiting for your feedback too.

😃

huonw · 2013-12-18T22:31:57Z

src/libextra/stats.rs

+            let mut j = 0;
+            // This inner loop applies `hi`/`lo` summation to each
+            // partial so that the list of partial sums remains exact.
+            while i < partials.len() {


This can just be for i in range(0, partials.len()).

Got so much to learn. Thanks for your review, though.
;-)

huonw · 2013-12-18T22:50:57Z

Looks good, however there are still some deficiencies with "extreme" values. In the following example, all print nan (the comment on the right is the correct value):

let nums = [
     &[f64::NAN, 1.0], // nan
     &[f64::INFINITY, 1.0], // inf
     &[1e308, 1e308, -1e308], // intermediate overflow, 1e308
     &[1e308, 1e308], // inf
     &[0.0, f64::NEG_INFINITY]]; // -inf
for v in nums.iter() {
    println!("{}", v.sum());
}

See this comment for a way to fix these; however, I think I'm ok with approving this if you open an issue saying "Stats.sum doesn't handle NaNs, infinities and overflow correctly" with the links from my comment and leave a comment like // FIXME #the_number_of_that_bug handle NaN, inf and overflow at the top of the method implementation.

huonw · 2013-12-18T22:56:00Z

Also, for merging, we'll want this a single commit, which you can do by squashing them.

huonw · 2013-12-19T00:07:04Z

src/libextra/stats.rs

+        let mut partials : ~[f64] = ~[];
+
+        for &mut x in self.iter() {
+            let mut i = 0;


Now that you're using range you don't need this separate i variable.

`[1e20, 1.0, -1e20].sum()` returns `0.0`. This happens because during the summation, `1.0` is too small relative to `1e20`, making it negligible. I have tried Kahan summation but it hasn't fixed the problem. Therefore, I've used Python's `fsum()` implementation with some help from Jason Fager and Huon Wilson. For more details, read: www.cs.cmu.edu/~quake-papers/robust-arithmetic.ps Moreover, benchmark and unit tests were added. Note: `Status.sum` is still not fully fixed. It doesn't handle NaNs, infinities and overflow correctly. See issue 11059: rust-lang#11059

@huonw

`[1e20, 1.0, -1e20].sum()` returns `0.0`. This happens because during the summation, `1.0` is too small relative to `1e20`, making it negligible. I have tried Kahan summation but it hasn't fixed the problem. Therefore, I've used Python's `fsum()` implementation. For more details, read: www.cs.cmu.edu/~quake-papers/robust-arithmetic.ps #10851 Python's fsum (msum) http://code.activestate.com/recipes/393090/ @huonw, your feedback is more than welcome. It looks unpolished; Do you have suggestions how to make it more beautiful and elegant? Thanks in advance,

Ignore more type aliases in unnecessary_cast Fixes rust-lang#10555 changelog: [`unnecessary_cast`]: No longer lints cast from locals that are type aliases

huonw reviewed Dec 12, 2013
View reviewed changes

huonw reviewed Dec 15, 2013
View reviewed changes

huonw reviewed Dec 18, 2013
View reviewed changes

g3xzh mentioned this pull request Dec 18, 2013

Stats.sum doesn't handle NaNs, infinities and overflow correctly #11059

Closed

huonw reviewed Dec 19, 2013
View reviewed changes

bors closed this Dec 19, 2013

bors merged commit 05395cb into rust-lang:master Dec 19, 2013

huonw mentioned this pull request Dec 19, 2013

extra::stats suffers from catastrophic cancellation #10851

Closed

g3xzh deleted the sum_bugfix branch December 25, 2013 00:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `sum()` accuracy #10927

Fix `sum()` accuracy #10927

g3xzh commented Dec 12, 2013

huonw Dec 12, 2013

g3xzh commented Dec 12, 2013

huonw Dec 12, 2013

huonw commented Dec 12, 2013

jfager commented Dec 14, 2013

huonw commented Dec 14, 2013

jfager commented Dec 14, 2013

huonw commented Dec 14, 2013

jfager commented Dec 14, 2013

huonw Dec 15, 2013

g3xzh commented Dec 18, 2013

huonw Dec 18, 2013

g3xzh Dec 18, 2013

huonw commented Dec 18, 2013

huonw commented Dec 18, 2013

huonw Dec 19, 2013

Fix sum() accuracy #10927

Fix sum() accuracy #10927

Conversation

g3xzh commented Dec 12, 2013

huonw Dec 12, 2013

Choose a reason for hiding this comment

g3xzh commented Dec 12, 2013

huonw Dec 12, 2013

Choose a reason for hiding this comment

huonw commented Dec 12, 2013

jfager commented Dec 14, 2013

huonw commented Dec 14, 2013

jfager commented Dec 14, 2013

huonw commented Dec 14, 2013

jfager commented Dec 14, 2013

huonw Dec 15, 2013

Choose a reason for hiding this comment

g3xzh commented Dec 18, 2013

huonw Dec 18, 2013

Choose a reason for hiding this comment

g3xzh Dec 18, 2013

Choose a reason for hiding this comment

huonw commented Dec 18, 2013

huonw commented Dec 18, 2013

huonw Dec 19, 2013

Choose a reason for hiding this comment

Fix `sum()` accuracy #10927

Fix `sum()` accuracy #10927