Skip to content
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.

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

Merge 0.5 #599

Merged
merged 6 commits into from
Sep 24, 2018
Merged

Merge 0.5 #599

merged 6 commits into from
Sep 24, 2018

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Sep 3, 2018

No description provided.

@dhardy
Copy link
Member Author

dhardy commented Sep 4, 2018

The CI still fails — apparently most of the Travis workers don't get the same result I do (can't reproduce).

Rand still depends on rand_core 0.2.0 despite this feature being in an un-released version. Not a problem for CI (it uses the repo paths), but we should release rand_core 0.3 and increase the dependency version.

@dhardy
Copy link
Member Author

dhardy commented Sep 20, 2018

@sicking I've been trying to work out why one of the tests is generating anomalous results and failing, but I think it's just a random failure (I double-checked the code). This is the "counts" results with std (passes):

chosen: [110, 99, 122, 120, 104, 106, 122, 122, 95]
chosen: [132, 130, 100, 125, 111, 103, 105, 92, 102]
chosen: [107, 103, 135, 110, 110, 107, 115, 112, 101]
chosen: [109, 112, 108, 109, 105, 103, 140, 105, 109]
chosen: [115, 109, 104, 120, 104, 125, 104, 114, 105]
chosen: [114, 98, 130, 117, 107, 119, 127, 93, 95]
chosen: [106, 106, 119, 100, 119, 120, 103, 115, 112]
chosen: [107, 96, 113, 117, 116, 95, 119, 118, 119]

And without (one test skipped, only up to failing test with 153):

chosen: [110, 99, 122, 120, 104, 106, 122, 122, 95]
chosen: [132, 130, 100, 125, 111, 103, 105, 92, 102]
chosen: [97, 136, 117, 114, 127, 108, 94, 120, 87]
chosen: [107, 119, 103, 105, 104, 153, 106, 102, 101]

I've also adjusted your code slightly — see comments in #593.

Do you think this is reasonable?

@dhardy dhardy added the D-review Do: needs review label Sep 21, 2018
@dhardy dhardy mentioned this pull request Sep 22, 2018
28 tasks
benches/seq.rs Outdated
@@ -127,7 +127,7 @@ fn seq_iter_unhinted_choose_from_100(b: &mut Bencher) {
#[bench]
fn seq_iter_window_hinted_choose_from_100(b: &mut Bencher) {
let mut rng = SmallRng::from_rng(thread_rng()).unwrap();
let x : &[usize] = &[1; 100];
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably want to change the name of the benchmark function.

src/seq/mod.rs Outdated
assert!(-30 <= err && err <= 30);
// Octave: binopdf(x, 1000, 1/9) gives the prob of *count == x
// Note: have seen 153, which is unlikely but not impossible.
assert!(72 < *count && *count < 154, "count not close to 1000/9: {}", count);
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems fine. The old limits had no scientific analysis behind them. I mainly picked as small of a number as I could get away with.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was actually surprised I had to go this high — binopdf(153, 1000, 1/9) is 1.1e-5. But not impossible and I'm fairly sure the algorithm is right.

consumed = 1;
let hint = self.size_hint();
lower = hint.0;
upper = hint.1;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong opinion on this one. I answered over in #593. In theory this change will slow down unhinted iterators, but speed up iterators with "unusual" hinting (such as the "window hinted" one in the tests). But in both cases the change should be quite small. And of course less code is always a win.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's difficult tying much to theory when it comes to optimisations isn't it? Perhaps simpler code works better with the branch predictor. It's actually the unhinted one that's faster.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. I agree that using too much theory for optimizations can be dangerous. But this is likewise true for using microbenchmarks.

Either way, the intended win from this code is really small, so I'm totally ok with it being removed.

@@ -191,17 +191,7 @@ pub trait IteratorRandom: Iterator + Sized {
let mut result = None;

if upper == Some(lower) {
// Remove this once we can specialize on ExactSizeIterator
return if lower == 0 { None } else { self.nth(rng.gen_range(0, lower)) };
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, removing the comment is probably a good idea. See for example this for a good reason we probably will want to keep this code around no matter what: rust-lang/rust#34433

@dhardy dhardy merged commit 4685924 into rust-random:master Sep 24, 2018
@dhardy dhardy deleted the merge-0.5 branch September 24, 2018 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D-review Do: needs review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants