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

Update to rand >= 0.8 #133

Closed
rob-p opened this issue Apr 12, 2021 · 6 comments · Fixed by #134
Closed

Update to rand >= 0.8 #133

rob-p opened this issue Apr 12, 2021 · 6 comments · Fixed by #134

Comments

@rob-p
Copy link

rob-p commented Apr 12, 2021

Currently, statrs relies on rand 0.7 and nalgebra 0.23 (which itself relies on rand 0.7). The newer releases of nalgebra update their dependency to the latest rand, which is currently 0.8.3. One of the minor, but frustrating, differences between 0.7 and 0.8 is a change in the syntax of the rand_range function, from taking 2 arguments to taking a single range argument.

It would be great if statrs could be updated to rely on a newer nalgebra and a newer statrs. Currently, if one is depending on rand >= 0.8 (or is depending on any package that depends on this), then multiple different versions of rand are pulled down. Not only does this bloat the build, but it can run the risk of confusing the compiler about definitions that appear in both versions of the package.

The actual changes to conform with the new API interface of rnd_range are small, but I think there are some other changes that would need to be made, since the version of nalgebra should be bumped (but perhaps not to the very latest (0.26.1), unless other changes are made because they have deprecated some interfaces that are currently used in statrs).

@vks
Copy link
Contributor

vks commented Apr 12, 2021

but it can run the risk of confusing the compiler about definitions that appear in both versions of the package.

I don't think this can happen. There are some crates for which you can only link to one version, but rand is not one of them.

@rob-p
Copy link
Author

rob-p commented Apr 13, 2021

I noticed this behavior when trying to do the update myself. I altered statrs to properly use rand 0.8 (changes were minimal), but then the nalgebra dependency pulled in rand 0.7. This confused the compiler abour the definition of the sample function, and prevented compilation.

@vks vks mentioned this issue Apr 13, 2021
@vks
Copy link
Contributor

vks commented Apr 13, 2021

I noticed this behavior when trying to do the update myself. I altered statrs to properly use rand 0.8 (changes were minimal), but then the nalgebra dependency pulled in rand 0.7. This confused the compiler abour the definition of the sample function, and prevented compilation.

This sounds like a legitimate type mismatch. nalgebra and statrs have rand types in their public APIs, therefore they have to use the same crate (the same version of rand) to be compatible.

@rob-p
Copy link
Author

rob-p commented Apr 13, 2021

That may very well be the case. I found the compiler message to be a bit cryptic, even though it was clever enough to suggest

note: perhaps two different versions of crate `rand` are being used?

In particular, the sample method that I thought should be used was the one in statrs's normal.rs, which specializes the implementation for the Normal struct (rather than the generic implementation provided by rand). However, the compiler seemed to not want to apply this impl and instead was reaching for the one provided by rand.

@vks
Copy link
Contributor

vks commented Apr 13, 2021

Yes, unfortunately the compiler messages can be very confusing in such cases! This is an old issue, see rust-lang/rust#22750.

The compiler does not now about crate versions (that's a concept only cargo knows about), so for the compiler it looks like you are mixing two different types from possibly completely unrelated crates. What makes it even more confusing, is that the compiler usually shortens the types (to make the error message more readable), but in such cases this can result in messages like "Expected Distribution, got Distribution instead", where the two Distribution types are from different crates (e.g. in this case two different rand versions).

@rob-p
Copy link
Author

rob-p commented Apr 13, 2021

Oh wow; that is confusing. Anyway, thanks for the details, the pointer, and the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants