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.

Sign up for GitHub

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

Jump to bottom

WeightedChoice is hard to use with borrow checker #142

Closed
Binero opened this issue Mar 11, 2017 · 2 comments
Closed

WeightedChoice is hard to use with borrow checker #142

Binero opened this issue Mar 11, 2017 · 2 comments
Labels
F-new-int Functionality: new, within Rand

Comments

@Binero
Copy link

Binero commented Mar 11, 2017

WeightedChoice is quite hard to use with Rust's borrow checker.

To create a WeightedChoice you need to give it an &'mut [Weighted]. This means that your [Weighted] is unavailable until WeightedChoice goes out of scope.

This makes it impossible to store a WeightedChoice in a structure, or return it from a function. Both scenarios would require you to also store or return your [Weighted], which is impossible because it's also borrowed by WeightedChoice.

As such, it's only possible to use a WeightedChoice inside the same scope that created the [Weighted], which means it is impossible to hide it away in an API.

It doesn't seem however, that WeightedChoice actually needs a mutable reference to the [Weighted]. It only needs this during its construction.
@mbrubeck
Copy link
Contributor

Related: #90.

@mbrubeck mbrubeck mentioned this issue May 22, 2017
Closed
@ramn ramn mentioned this issue May 22, 2017
Closed
@alexcrichton alexcrichton added the F-new-int Functionality: new, within Rand label Jun 14, 2017
dhardy added a commit to dhardy/rand that referenced this issue Jul 29, 2017
@dhardy
Rename Sample → Distribution. Make WeightedChoice own its arguments.
0df5fd1 
The change to WeightedChoice is debatable, but since the argument must normally
be constructed specially anyway, the reference model makes little sense.
See issues rust-random#90, rust-random#142 and PR rust-random#152.
@dhardy dhardy mentioned this issue Jan 6, 2018
Closed
5 tasks
rfdonnelly added a commit to rfdonnelly/rvs that referenced this issue Jan 8, 2018
@rfdonnelly
Use released version of rand instead of experimental
0a66133 
Notable changes:

* transform::rand::Seed got simpler
* Misc minor API differences
* Swap out rand::distributions::WeightedChoice with custom
  WeightedIndexes type

  WeightedChoice for released version of rand does not own the array it
  operates on.  Because of this, it is not possible to keep around an
  instance of WeightedChoice and reuse its internal range.  The
  experimental version of rand resolves this but it cannot be used by
  published crates.  Fix by rolling our own WeightedIndexes type.

  See rand issues:
  * rust-random/rand#90
  * rust-random/rand#142
@sicking
Copy link
Contributor

sicking commented Jul 22, 2018

This API was deprecated in #518 and the new API seems to be doing better here.

@dhardy dhardy closed this as completed Jul 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
F-new-int Functionality: new, within Rand
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add an owned variant of WeightedChoice. mbrubeck/rand
5 participants
@mbrubeck @alexcrichton @dhardy @sicking @Binero