Skip to content

Add randn for arrays of standard gaussian distributed random numbers. #146

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

Closed

Conversation

daniel-vainsencher
Copy link
Contributor

This time trying to rebase, rather than merge, let me know if I've done it correctly.

bluss and others added 30 commits March 12, 2016 15:08
We don't want to compile crate ndarray twice for benchmarks and tests;
only once. Tests and benchmarks are outside of the crate.
The new ShapeError is a struct, so that we can stuff more information
into it without changing the API.
A .push() loop is not efficient enough; we can do this manually using
our knowledge that we will produce exactly as many elements as was
preallocated for.
The former will optimize out in some cases where the other won't.
Improves `dot` bench in fact.
`Debug` enables much better error messages. We control all implementors,
so this is a very safe change.
We're in quick development, so we are sticking with the latest stable rust.
@bluss
Copy link
Member

bluss commented Mar 12, 2016

Thanks. I'm not promising that we will integrate rand directly like this. It could for example belong to a crate of its own "ndarray-rand" or so.
Something is definitely amiss when there are so many commits listed in the PR. Would be great if you could reset your branch to just contain one patch on top of master and force push again.

@daniel-vainsencher
Copy link
Contributor Author

This is just a first cut. I agree we need to get the dependencies and design right. And I'll have no choice but to read up on git rebasing... out of time for now, will get back to this, as I need it. BTW, is there a good alternative to depending on rand?

@bluss
Copy link
Member

bluss commented Mar 12, 2016

It's just one commit, so you can use cherry-pick to move it to the correct branch. It's simple, like this #124 (comment)

I don't know a better crate than rand.

rand could be an optional dependency, but even better than that (also what crates.io advises) is to make a separate crate if the feature is easy to split out.

A new crate ndarray-rand could be hosted by me, it would depend on ndarray and rand, and it provides these constructors.

@bluss
Copy link
Member

bluss commented Mar 12, 2016

Maybe using rand for the gaussian samples is the wrong thing? I wonder if we can do this much more efficiently if we're generating a big set.

@daniel-vainsencher
Copy link
Contributor Author

I am not sure what is the significant advantage of avoiding rand as a dependency of ndarray.

  • If we create our own code that takes advantage of making a whole array of them at a time, that's fine, but currently a hypothetical.
  • Otherwise random numbers are not esoteric in numerical computing, they are a fundamental tool used all the time. randn is one of my most common numpy imports in python.
  • While cargo is good enough that having users add another crate is not difficult, this only works if they are aware it exists; discovery is still made harder. I haven't thought through any ergonomic differences for users of randn.

@daniel-vainsencher
Copy link
Contributor Author

Created a new pull request with clean history and addressing the points above.

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 this pull request may close these issues.

4 participants