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

Re-export rand distributions #663

Merged

Conversation

LukeMathWalker
Copy link
Member

@LukeMathWalker LukeMathWalker commented Jul 20, 2019

This is what I meant in #659 @max-sixty.

I have tested it on my linear regression example - it runs without any issue, even if I explicitly specify rand = 0.7 in Cargo.toml.

@max-sixty
Copy link
Contributor

Great, thanks @LukeMathWalker

@jturner314
Copy link
Member

I'm in favor of trying to prevent confusion like #530 and #658 from ocurring again, but I'm not sure that this is the right approach. If we do this, should we also re-export the Rng trait used by RandomExt::random_using? If we do that, should we re-export RNG creation routines?

Here are a few options:

  1. Re-export all of rand. This works okay for stuff in rand, but it could be confusing when people want to use other rand-dependent crates too (e.g. rand_isaac for the ISAAC RNGs).

  2. Use the rename-dependency trick I proposed in Add conversions from nalgebra types #554 (to support conversions between ndarray's types and multiple versions of nalgebra) to support multiple versions of rand. So, we'd have RandomExt_0_6, RandomExt_0_7, etc. In addition to the benefit of supporting multiple versions, the names of the traits would make it clear which version of rand would be required to use them.

  3. Update ndarray-rand to rand 0.7 and clearly indicate in the docs that it uses rand 0.7.

Personally, I prefer option 3 because it's the simplest and because I don't see much benefit to supporting multiple versions of rand, but I'm open to other options.

Fwiw, when I use ndarray-rand, I pretty much always pick a specific RNG and seed it from a known value so that I get reproducible results. This means that I need to use a RNG from a separate crate (e.g. rand_isaac::Isaac64Rng), the rand::SeedableRng trait, the relevant Distribution type, and ndarray_rand::RandomExt.

@LukeMathWalker
Copy link
Member Author

LukeMathWalker commented Jul 22, 2019 via email

@jturner314
Copy link
Member

I would personally go for both 1. and 3. - at least until rand reaches 1.0. We can vendor it in a ndarray_rand::rand submodule and it would provide a confusion-free experience for most of the people using the crate.

Okay, that sounds good to me. Thanks for looking into this.

@jturner314
Copy link
Member

Should we re-export rand_distr in addition to rand since most of rand::distributions is deprecated?

@LukeMathWalker
Copy link
Member Author

Should we re-export rand_distr in addition to rand since most of rand::distributions is deprecated?

I'd say it makes sense to do so.
I am out in the woods these days, so I won't be able to fix this until I am back home (August 4th). If we want to bundle it with #659 and cut a release sooner than that feel free to take over 😅

@LukeMathWalker
Copy link
Member Author

Ok, this should be ready now:

  • I re-exported rand-distr and adapted the docs;
  • I added a new example to random_using showing the workflow with a seeded rng (Isaac64)
  • I converted the README to Markdown format for crates.io consumption (with an example)
  • I moved the changelog to a separate file, keeping the last release changelog in the README.md

Once this gets merged I think we could cut a release (or should we wait on #670 and just release everything at once?).

@bluss
Copy link
Member

bluss commented Sep 3, 2019

Seems reasonable, although I don't see a written motivation for why we should depend on rand_distr (since it's an optional extra), but it seems reasonable.

An important note though, with rename-dependency if it works well, we could move rand support directly into ndarray itself, since we can risk taking on a public dependency - since we can add feature flags for new versions of rand.

It's still a burden and a risk - we risk having to support many rand versions per ndarray version.

If it would work, ndarray 0.13 (next version) could be released with a feature called rand_0_7, and one could add a rand_0_8 feature later during the ndarray 0.13.x cycle if required. It's a bit messy, and I'm not sure users would enjoy to have traits called ndarray::RandomExt07 or similar? But it's hard to avoid the naming problem if we have multiple parallel possible implementations.

One way to avoid the parallel names would be if we could "key" the trait by a type or trait from rand. Like the trait ndarray::RandomExt<T> would automatically be different if it was parameterized by a type T that's from rand 0.7, rand 0.8 etc respectively.

@LukeMathWalker
Copy link
Member Author

LukeMathWalker commented Sep 4, 2019

It's still a burden and a risk - we risk having to support many rand versions per ndarray version.

I am little scared by this - I don't have good visibility over rand and how "far" they are from a stable 1.0 release. We might end up piling up a lot of different implementations without gaining too much from it.
It might be simpler to start out supporting only a single version (the latest) and then see how that plays out.

@bluss
Copy link
Member

bluss commented Sep 9, 2019

When I envison the parallel rand features, it just doesn't work out for our use case. We'd need some way to make it work transparently without resorting to parallel RandExt07, RandExt08 traits etc, that would just move the problem to a different place.

Oh and i noticed that ndarray-rand is still not published with a new version.. What's holding this back? Is it just that the releasers are not paying attention? (That's me, by the way)

@bluss
Copy link
Member

bluss commented Sep 9, 2019

Ok, so I initially agree with jturner that reexporting is not a great solution.

The only way I can see it as a good solution is if you have a project that's very ndarray focused, and only consuming rand through ndarray itself, and no other ways?

I have a hard time deciding, but I think this is an ok solution, when I realize that using the reexports is optional. Users can use the crate directly if they want, and hopefully this will be clear.

With that said, I'd like to get this merged so that we can release ndarray-rand with rand 0.7 support soon.

ndarray-rand/src/lib.rs Outdated Show resolved Hide resolved
@jturner314
Copy link
Member

Everything looks good to me except for the extern crate lines that @bluss pointed out (and the similar extern crate lines elsewhere).

@LukeMathWalker
Copy link
Member Author

I have a hard time deciding, but I think this is an ok solution, when I realize that using the reexports is optional. Users can use the crate directly if they want, and hopefully this will be clear.

In the end, it's up to the user to use it, we are not forcing it on them.
I'll fix the extern crate lines, so that we can move forward with it.

ndarray-rand/README.md Outdated Show resolved Hide resolved
ndarray-rand/README.md Outdated Show resolved Hide resolved

use ndarray::ShapeBuilder;
use ndarray::{ArrayBase, DataOwned, Dimension};

/// [`rand`](https://docs.rs/rand/0.7.0/rand/), re-exported for convenience and version-compatibility.
pub mod rand {
Copy link
Member

Choose a reason for hiding this comment

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

Might be my ignorance, I thought we had pub extern crate for re-exporting, but I'm unsure what difference it makes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Never heard of pub extern crate <...>!
Digging around, it seems they have exactly the same effect ( nix-rust/nix#694 ).
Using mod has the plus of being more explicit in my eyes, given that it uses the "standard" visibility mechanism of the 2018 edition.

@LukeMathWalker LukeMathWalker merged commit a364cc8 into rust-ndarray:master Sep 10, 2019
@LukeMathWalker LukeMathWalker deleted the re-export-rand-distribution branch September 10, 2019 07:38
@bluss
Copy link
Member

bluss commented Sep 10, 2019

Nice :) so I published the next version of ndarray-rand 0.10, so we finally have that out the door, sorry for the delay in that.

ndarray-rand might need to be careful about guaranteeing specific minor versions since we don't track or check it perfectly, when it's embedded in this repo. But it's such a small crate it might not matter.

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

Successfully merging this pull request may close these issues.

4 participants