You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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).
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.
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
changed the title
No usize
Rng::gen_index; no usize for Standard
Aug 10, 2024
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CHANGELOG.mdentrySummary
Follows #1470, hence the first commit of this PR is "cfg-gate IndexVec::USize".
Motivation
Solve #1399 by removing support for sampling
usize,isizethrough 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:Standarddistribution support forusize,isizeand non-zero and SIMD variantsUniformdistribution support for the same typesFill-ing[usize],[isize]Rng::gen_indexWeightedAliasIndexno longer supportsisizeorusizeweightsThis 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_indexsupports various range types overusizeusing 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 justRangeTo; that's all thatranduses anyway.)We might also want to expose
UniformUsize(used inrand::distr::slice), probably underrand::distr::uniform(alongsideUniformInt, possibly without implementingSampleUniformthough that is an option).