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

Rng::gen_index; no usize for Standard #1471

Closed
wants to merge 10 commits into from
Closed

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Jul 19, 2024

  • Added a CHANGELOG.md entry

Summary

Follows #1470, hence the first commit of this PR is "cfg-gate IndexVec::USize".

Motivation

Solve #1399 by removing support for sampling usize, isize through the usual methods.

Details

This is potentially the most drastic and problematic approach, but seems to work fine at least for rand's tests. Specifically, it removes:

  • Standard distribution support for usize, isize and non-zero and SIMD variants
  • Uniform distribution support for the same types
  • Support for Fill-ing [usize], [isize]
  • Adds Rng::gen_index
  • WeightedAliasIndex no longer supports isize or usize weights

This is meant for discussion. Potentially we could also use in a release with the option to add back support for the above types (this would be non-breaking).

fn Rng::gen_index supports various range types over usize using a private trait bound. Unorthodox and it's possible that the lack of ability for downstream crates to write generics over this would be an issue, but that is really not an intended feature. (We could also simplify this down to just RangeTo; that's all that rand uses anyway.)

We might also want to expose UniformUsize (used in rand::distr::slice), probably under rand::distr::uniform (alongside UniformInt, possibly without implementing SampleUniform though that is an option).

@dhardy dhardy added the E-question Participation: opinions wanted label Jul 26, 2024
@dhardy dhardy marked this pull request as ready for review July 26, 2024 07:42
@dhardy dhardy requested review from newpavlov and vks July 26, 2024 07:42
@dhardy
Copy link
Member Author

dhardy commented Jul 27, 2024

I added UniformUsize (so Uniform supports usize). This uses 32-bit sampling where possible, which seems reasonable since in practice the high value will almost always be under u32::MAX. I haven't benchmarked against prior performance because I'm more interested in portability for the case where this is used.

isize for Uniform is omitted because I don't see any use-case (if there is, what is the expected behaviour?).

Fill and Standard still omit isize and usize support because... I can't think of a single reasonable usage, and further these are inherently non-portable.

@dhardy dhardy mentioned this pull request Jul 28, 2024
23 tasks
@dhardy dhardy requested a review from josephlr July 29, 2024 10:06
@dhardy dhardy added the D-review Do: needs review label Jul 29, 2024
@vks
Copy link
Collaborator

vks commented Jul 29, 2024

Do we still need Rng::gen_index if Rng::gen_range supports the same use case? (We would have to implement SampleUniform for UniformUsize.)

@dhardy
Copy link
Member Author

dhardy commented Jul 30, 2024

It turns out that due to the bounds on SampleBorrow, any implementor of SampleUniform must also implement SampleUniform.

Rng::gen_index supports ..end and ..=high which gen_range does not, but otherwise... no, we don't need gen_index. Perhaps removing that is the better option (less disruptive).

@dhardy dhardy changed the title No usize Rng::gen_index; no usize for Standard Aug 10, 2024
@dhardy dhardy mentioned this pull request Aug 12, 2024
1 task
@dhardy
Copy link
Member Author

dhardy commented Sep 10, 2024

#1487 was merged instead

@dhardy dhardy closed this Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D-review Do: needs review E-question Participation: opinions wanted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants