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

Implementing rand::Rand #14

Merged
merged 2 commits into from
Mar 6, 2018
Merged

Implementing rand::Rand #14

merged 2 commits into from
Mar 6, 2018

Conversation

shingtaklam1324
Copy link
Contributor

@shingtaklam1324 shingtaklam1324 commented Feb 13, 2018

Implements #12 .

Right now it is implemented with calling rng.gen::<T>() for both the real and imaginary parts. I'm not 100% sure on the mathematical randomness of the distribution if the complex number is used as a vector and the angle is wanted.

As well as that, for the dependency on rand, I just pulled the newest version on crates.io, but it may be better to use another version. I don't have many projects that use this so I'm not sure how big of an issue this will be.

Changes:

  • Optional rand feature and dependency on rand
  • impl<T> rand::Rand for Complex<T>

@cuviper
Copy link
Member

cuviper commented Feb 14, 2018

Thanks!

The distribution is an interesting question, but I think the simple way you've implemented it is the best we can do in general. If someone wants a smoother distribution of the angle, for instance, they can generate that separately and use from_polar.

We'll need to enable this feature in CI -- and if you do that for all targets you'll see that it won't work for rustc 1.8. Rand 0.4 requires rustc 1.15, and rand 0.3.22 now re-exports 0.4 too. I don't mind that this new optional feature isn't fully compatible though. I also intend to update rustc in a num-complex 0.2 bump very soon anyway.

Please do squash your commits -- you have a lot of unrelated churn. The final diff is fine, but I'd rather avoid altering the history of those other lines, affecting tools like git blame.

@shingtaklam1324
Copy link
Contributor Author

shingtaklam1324 commented Feb 15, 2018

@cuviper
I have squashed the commits as best as I can, but as a git noob, I can't seem to get it to do it properly.

As with the version of rand, I have it on = 0.3.14 right now as I can't get any other to compile on rustc 1.8.0

@cuviper
Copy link
Member

cuviper commented Mar 6, 2018

I rebased and squashed your commits completely, then updated to 0.4 since we're now using Rust 1.15 min.

Thanks again!

bors r+

bors bot added a commit that referenced this pull request Mar 6, 2018
14: Implementing rand::Rand r=cuviper a=shingtaklam1324

Implements #12 .

Right now it is implemented with calling `rng.gen::<T>()` for both the real and imaginary parts. I'm not 100% sure on the mathematical randomness of the distribution if the complex number is used as a vector and the angle is wanted.

As well as that, for the dependency on `rand`, I just pulled the newest version on crates.io, but it may be better to use another version. I don't have many projects that use this so I'm not sure how big of an issue this will be.

Changes:

- Optional `rand` feature and dependency on `rand`
- `impl<T> rand::Rand for Complex<T>`
@bors
Copy link
Contributor

bors bot commented Mar 6, 2018

Build succeeded

@bors bors bot merged commit 1a2449c into rust-num:master Mar 6, 2018
shingtaklam1324 added a commit to shingtaklam1324/num-complex that referenced this pull request Mar 7, 2018
@cuviper cuviper mentioned this pull request Mar 9, 2018
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.

2 participants