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

Create a distribution by mapping the output of another one #1129

Merged
merged 1 commit into from
May 24, 2021
Merged

Create a distribution by mapping the output of another one #1129

merged 1 commit into from
May 24, 2021

Conversation

adamreichold
Copy link
Contributor

This is useful if consumers are to be given an opaque type implementing the Distribution trait, but the output of the provided implementations needs additional post processing, e.g. to attach compile time units of measurement.

@vks
Copy link
Collaborator

vks commented May 23, 2021

Does this have an advantage over dist.sample_iter().map(...)?

@adamreichold
Copy link
Contributor Author

adamreichold commented May 23, 2021

Does this have an advantage over dist.sample_iter().map(...)?

My main use case is if a consumer needs a distribution producing a type that includes units of measurement, e.g. Distribution<uom::si::f64::Length> which I can create with distance_in_meters.map(|val| uom::si::f64::Length::new::<uom::si::length::meter>(val)) when distance_in_meters: Distribution<f64>.

If I changed the requirement of the consumer from Distribution<T> to Iterator<Item=T>, this would force me to keep the PRNG instance borrowed for the whole duration the consumer exists and it would be rather counter intuitive for someone reading the code as the fact the parameter represents a random variable is obscured. As I am using this for simulation models which need to be as easy to explain/understand as possible, these limitations would be significant issues for me.

@adamreichold
Copy link
Contributor Author

adamreichold commented May 23, 2021

this would force me to keep the PRNG instance borrowed for the whole duration the consumer exists

I am actually not sure whether I could fulfil this at all: I currently store the distributions with my model parameters while the PRNG state is part of the model state. The above would imply that I have to make these parameters part of the model state. Additionally, I would have to split the RNG streams for each distribution as I cannot mutably borrow a single one. And I would have to do this again for each agent as those are relocated between processes.

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not opposed to the idea. Code looks fine to me.

src/distributions/mod.rs Show resolved Hide resolved
src/distributions/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me. @vks can merge if he agrees.

@dhardy
Copy link
Member

dhardy commented May 23, 2021

Eratta: please add a note to the changelog.

This is useful if consumers are to be given an opaque type implementing the
Distribution trait, but the output of the provided implementations needs
additional post processing, e.g. to attach compile time units of measurement.
@adamreichold
Copy link
Contributor Author

Eratta: please add a note to the changelog.

Done.

@adamreichold
Copy link
Contributor Author

The CI failure looks related to packed_simd and I think unrelated to these changes. Is there anything I should do about this?

@dhardy
Copy link
Member

dhardy commented May 24, 2021

No. Unstable features break sometimes (no surprise).

@dhardy dhardy merged commit a97d94a into rust-random:master May 24, 2021
@adamreichold adamreichold deleted the dist-map branch May 24, 2021 07:02
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.

None yet

3 participants