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

Replace Floaty with num-traits #95

Closed
bheisler opened this issue Dec 15, 2017 · 4 comments · Fixed by #100
Closed

Replace Floaty with num-traits #95

bheisler opened this issue Dec 15, 2017 · 4 comments · Fixed by #100
Labels
Beginner Suitable for people new to Criterion.rs Enhancement

Comments

@bheisler
Copy link
Owner

bheisler commented Dec 15, 2017

Floaty doesn't seem to be maintained anymore. num-traits seems to be the standard for this sort of thing. I think it should just be a matter of updating all of the trait bounds and imports in stats.

Difficulty: Easy

@bheisler bheisler added Enhancement Beginner Suitable for people new to Criterion.rs labels Dec 15, 2017
@babbageclunk
Copy link
Contributor

I had a go at this (as someone who doesn't really know Rust), and the problem I'm hitting is that the floaty From trait has some cleverness to list which types can be safely promoted to floats (and so don't need Options around the result: http://japaric.github.io/float.rs/src/cast/lib.rs.html#253

That means that lots of the uses of Floaty::cast in the stats crate return Floaty.

As far as I can tell num-traits doesn't have this - the ::from method always returns an Option so I'm having to sprinkle lots of .unwrap()s in to compile (which I'm sure is wrong). What's the right way to do this?

Hmm, also num_traits::float::Float isn't Sync, so the threads in univariate and bivariate can't share them. There's an issue to implement it though: rust-num/num-traits#8
Maybe num_traits isn't ready to replace Floaty yet?

@bheisler
Copy link
Owner Author

Hmm. That's unfortunate, I thought num-traits was going to be more or less a drop-in-replacement.

For the options, I think it should be mostly safe to just unwrap them, but it's a bit ugly. You could perhaps try to add a function (or potentially a trait) to keep all of the cast-and-unwrap code in one place until something equivalent is added to num-traits.

As for the Sync, I think it should be possible to add an explict Sync bound to the generics for that function. Again, it can be removed later, when num-traits catches up.

Honestly, num-traits probably isn't ready to replace Floaty yet, but it is actively maintained and doesn't require nightly like Floaty does. I think the best option for now is to make the switch anyway and try to make it so we can remove the workarounds easily later.

Thanks for your help.

@bheisler
Copy link
Owner Author

Having thought about it some more, I think a good way to do this would be to add a new trait (stats::float::Float maybe) which extends num-traits' Float and adds the Sync bound and the necessary safe casts and change all of the stats code to use that instead. That way we can rely on num-traits for most of the code and it will be easy to replace our extension trait with num-traits' Float when they provide everything that Criterion needs.

@babbageclunk
Copy link
Contributor

Yeah, that makes sense, I'll have a go at that sometime soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Beginner Suitable for people new to Criterion.rs Enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants