Add NaNs, infinities and overflows support to sum #12355

g3xzh · 2014-02-17T23:46:02Z

Stats::sum handles NaNs and Infinities as elements in the list.
In addition to that, it also detects overflows.

@huonw - your review, please. :-)

Attempting to fix #11059

(BTW, thanks to @cmr, @sfackler, dybt, @bjz, huon, eibwen for the support on the IRC)

huonw · 2014-02-18T00:24:48Z

src/libextra/stats.rs


+        fn samesign(x:f64, y:f64) -> bool {


Style is x: f64 with a space.

huonw · 2014-02-18T00:36:00Z

Looks pretty good, thanks!

adrientetar · 2014-02-18T11:36:58Z

src/libextra/stats.rs

+                    let mut lo = l;
+                    if hi.is_infinite() {
+                        let mut sign = -1f64;
+                        if hi > 0f64 { sign = 1f64 }


This could be written:

let sign = if hi > 0f64 { 1f64 } else { -1f64 };

Spares an alloc in the else case and a mut annotation.

There's no allocation here ;P and LLVM will almost certainly optimise to the same code. But I agree that if hi > 0.0 {1.0} else {-1.0} would be better.

(Also, @g3xzh, note that you can avoid the verbose/ugly f64 suffix by using a generic 0.0 literal. :) )

adrientetar · 2014-02-18T11:38:49Z

Why is there a distinct sum function of the one in std::num?

huonw · 2014-02-18T11:41:43Z

There is no sum in std::num... but this fancier function is necessary to avoid the catastrophic cancellation that occurs in the naive algorithm of just +ing everything. e.g. the snippet in #10851.

adrientetar · 2014-02-18T11:50:10Z

@huonw Then why isn't this part of the stdlib?

huonw · 2014-02-18T11:52:53Z

It kinda is in the stdlib here.

(It is significantly slower than just straight + (e.g. it cannot be vectorised), and many places do not need to worry about numbers varying wildly. However moving it somewhere else would also be ok by me; but it's out-of-scope for this PR, which is just completing the implementation.)

adrientetar · 2014-02-18T11:55:33Z

Sure. I think we will want IEEE 754-2008 in std at some point through.

huonw · 2014-02-18T12:02:47Z

AFAICT, I don't think IEEE 754 includes an accurate summation algorithm in the spec.

g3xzh · 2014-02-18T18:28:45Z

@huonw and @adrientetar - I would like to read your feedbacks.
Thanks in advance.

huonw · 2014-02-18T21:13:25Z

src/libextra/stats.rs

+        if partials[0] == 0.0 {
+            let mut tmp_pop = partials.pop();
+            if tmp_pop.is_none() { fail!("stats::sum() failure"); }
+            let mut total_so_far = tmp_pop.unwrap();


You can write

let mut total_so_far = partials.pop().expect("Impossible empty vector in Stats.sum");

which does the .is_none check + fail! internally.

Looks great. 👍

huonw · 2014-02-23T13:59:25Z

This needs a rebase too.

g3xzh · 2014-02-23T21:08:48Z

@huonw, 🏁 rebased.

huonw · 2014-02-23T21:12:52Z

GitHub is still saying it needs a rebase. You'll probably need to fetch mozilla/rust again.

adrientetar · 2014-02-23T21:14:05Z

@huonw How can you tell?

huonw · 2014-02-23T21:15:53Z

@adrientetar github shows committers (and only commiters :( ) in a little box above this comment box. The "Stale" category of the bors queue shows it too (but bors only runs every 5 minutes, so that's on a bit of a delay).

huonw · 2014-02-28T13:20:36Z

Yes, please (I'll have one final look over tomorrow morning but I think this is good to go).

Stats::sum handles NaNs and infinities as elements in the list. In addition to that, it also detects overflows. Note: Stats::sum is using std::vec_ng:Vec to improve its performance.

g3xzh · 2014-03-01T23:28:31Z

@huonw - done.

g3xzh · 2014-03-03T01:16:52Z

@huonw , is there a way to debug it without installing a 32bit linux (on a VM)?

huonw · 2014-03-03T02:12:12Z

The test definitely passes locally for you?

g3xzh · 2014-03-03T02:35:17Z

That is correct. it passes locally:

$ uname -a
Linux htpz 3.11.0-17-generic #31-Ubuntu SMP Mon Feb 3 21:52:43 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux

huonw · 2014-03-03T03:38:41Z

I guess installing a 32bit linux in a VM would work. (Theoretically you could use one of the distributions with nightly packages to just compile the module with tests directly, rather than doing the full bootstrap.)

emberian · 2014-03-03T04:00:57Z

Presumably you could also just cross-compile, ./configure
--target=x86_64-unknown-linux-gnu,i686-unknown-linux-gnu

On Sun, Mar 2, 2014 at 10:38 PM, Huon Wilson notifications@github.comwrote:

I guess installing a 32bit linux in a VM would work. (Theoretically you
could use one of the nightly packageshttps://github.com/mozilla/rust/wiki/Doc-packages,-editors,-and-other-toolsto just compile the module with tests directly, rather than doing the full
bootstrap.)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/12355#issuecomment-36479383
.

http://octayn.net/

g3xzh · 2014-03-03T19:07:30Z

I can't reproduce it.

vmuser1@vbox:~/rust/src/libextra$ ./extra sum_

running 5 tests
test stats::tests::test_sum_f64_between_ints_that_sum_to_0 ... ok
test stats::tests::test_sum_f64s ... ok
test stats::tests::test_sum_overflow_64 ... ok
test stats::bench::sum_many_f64 ... ignored
test stats::bench::sum_three_items ... ignored

test result: ok. 3 passed; 0 failed; 2 ignored; 0 measured

vmuser1@vbox:~/rust/src/libextra$ uname -a
Linux vbox 3.8.0-19-generic #29-Ubuntu SMP Wed Apr 17 18:19:42 UTC 2013 i686 i686 i686 GNU/Linux

Any ideas?

sfackler · 2014-03-03T19:13:43Z

It looks like it failed on the nopt builder. Is your build still optimizing?

g3xzh · 2014-03-03T19:33:05Z

@sfackler - I have no idea. I compile by executing rustc --test lib.rs and then run the tests.

huonw · 2014-03-04T01:25:35Z

Huh, I just ran it locally on real x86 hardware... passes fine. I've got no idea what's up with this.

Maybe you could add some error!("{}", ...) prints to the test see what values are being computed?

g3xzh · 2014-03-04T20:53:15Z

@huonw lets do it! :)

thestinger · 2014-03-05T17:30:12Z

I don't really agree with the overflow handling here. It should really just become INFINITY or -INFINITY if it's following the IEEE754 semantics that are used elsewhere. It is indeed correct to handle NaN by propagating it upwards, but I don't think it should be special-cased. An extra branch in every iteration of the loop for an exceptional case instead of just letting to the regular IEEE754 semantics do the work for us is strange.

huonw · 2014-03-06T05:22:52Z

This is just using the same algorithm that Python was using (they now use a bigint based one, iirc), which comes with a proof of correctness. As such, I would suggest that we merge this without significant modification, and defer (considered) adjustments to later pull requests.

(I do agree with both of your points though.)

(@g3xzh I'll r+ later today, and kill the builds on most bots so that we can just see the results on the 32-bit ones.)

huonw · 2014-03-06T13:01:16Z

The exceptions are because I killed those builds because we're investigating this 32-bit failure. The following are still running:

huonw · 2014-03-06T13:38:38Z

And http://buildbot.rust-lang.org/builders/auto-linux-32-nopt-t/builds/3713/ failed with output:

metrics saved to: tmp/check-stage2-T-i686-unknown-linux-gnu-H-i686-unknown-linux-gnu-extra-metrics.json
failures:

---- stats::tests::test_sum_overflow_f64 stdout ----
    inf 
    inf 


failures:
    stats::tests::test_sum_overflow_f64

test result: FAILED. 60 passed; 1 failed; 0 ignored; 2 measured

g3xzh · 2014-03-08T20:37:05Z

Doesn't make sense. Why is f64::MAX_EXP an inf?
IMHO, it's not related to my changes. I can change the test but it doesn't feel right.
Any suggestions?

alexcrichton · 2014-03-18T21:08:45Z

Closing due to inactivity, feel free to reopen with a rebase!

New attempt to generalize stats, after #12606. Since #12355 did not get merged, i want go get first get my change done and the try to fix sum.

…rds, r=flodiebold Fix inference when pattern matching a tuple field with a wildcard This should fix the following issue: rust-lang/rust-analyzer#12331 * Replaced the `err_ty` in `infer_pat()` with a new type variable. * Had to change the iterator code a bit, to get around multiple mutable borrows of `self` in `infer_pat()`. Also added a test * Also added a test

[`box_default`]: Preserve required path segments When encountering code such as: ```rs Box::new(outer::Inner::default()) ``` clippy would suggest replacing with `Box::<Inner>::default()`, dropping the `outer::` segment. This behavior is incorrect and that commit fixes it. What it does is it checks the contents of the `Box::new` and, if it is of the form `A::B::default`, does a text replacement, inserting `A::B` in the `Box`'s quickfix generic list. If the source does not match that pattern (including `Vec::from(..)` or other `T::new()` calls), we then fallback to the original code. Fixes rust-lang#11927 *Please write a short comment explaining your change (or "none" for internal only changes)* changelog: [`box_default`]: Preserve required path segments

huonw reviewed Feb 18, 2014
View reviewed changes

src/libextra/stats.rs

fn samesign(x:f64, y:f64) -> bool {

Copy link

Member

huonw Feb 18, 2014

Choose a reason for hiding this comment

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

Style is x: f64 with a space.

adrientetar reviewed Feb 18, 2014
View reviewed changes

huonw reviewed Feb 18, 2014
View reviewed changes

huonw mentioned this pull request Feb 28, 2014

Generalize stats from f64 to the Float trait #12606

Closed

Add NaNs, infinities and overflows support to sum

2e00bb0

Stats::sum handles NaNs and infinities as elements in the list. In addition to that, it also detects overflows. Note: Stats::sum is using std::vec_ng:Vec to improve its performance.

Add error for debugging

0127d76

Fix indentation

e3e8c33

alexcrichton closed this Mar 18, 2014

EdorianDark mentioned this pull request Apr 28, 2014

Generalize stats from f64 to the Float trait #13822

Merged

bors added a commit that referenced this pull request May 6, 2014

auto merge of #13822 : EdorianDark/rust/master, r=cmr

1f6db7f

New attempt to generalize stats, after #12606. Since #12355 did not get merged, i want go get first get my change done and the try to fix sum.

Add NaNs, infinities and overflows support to sum #12355

Add NaNs, infinities and overflows support to sum #12355

Conversation

g3xzh commented Feb 17, 2014

huonw Feb 18, 2014

Choose a reason for hiding this comment

huonw commented Feb 18, 2014

adrientetar Feb 18, 2014

Choose a reason for hiding this comment

huonw Feb 18, 2014

Choose a reason for hiding this comment

g3xzh Feb 18, 2014

Choose a reason for hiding this comment

adrientetar commented Feb 18, 2014

huonw commented Feb 18, 2014

adrientetar commented Feb 18, 2014

huonw commented Feb 18, 2014

adrientetar commented Feb 18, 2014

huonw commented Feb 18, 2014

g3xzh commented Feb 18, 2014

huonw Feb 18, 2014

Choose a reason for hiding this comment

g3xzh Feb 18, 2014

Choose a reason for hiding this comment

huonw commented Feb 23, 2014

g3xzh commented Feb 23, 2014

huonw commented Feb 23, 2014

adrientetar commented Feb 23, 2014

huonw commented Feb 23, 2014

huonw commented Feb 28, 2014

g3xzh commented Mar 1, 2014

g3xzh commented Mar 3, 2014

huonw commented Mar 3, 2014

g3xzh commented Mar 3, 2014

huonw commented Mar 3, 2014

emberian commented Mar 3, 2014

g3xzh commented Mar 3, 2014

sfackler commented Mar 3, 2014

g3xzh commented Mar 3, 2014

huonw commented Mar 4, 2014

g3xzh commented Mar 4, 2014

thestinger commented Mar 5, 2014

huonw commented Mar 6, 2014

huonw commented Mar 6, 2014

huonw commented Mar 6, 2014

g3xzh commented Mar 8, 2014

alexcrichton commented Mar 18, 2014