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

Add note about floating point weights in update_weights docs #1280

Merged
merged 4 commits into from
Feb 2, 2023

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Jan 26, 2023

Motivation

To highlight that update_weights may not return WeightedError::AllWeightsZero when all weights are zero when updating floating-point weights.

@dhardy
Copy link
Member

dhardy commented Jan 31, 2023

Thanks for the PR. But I don't understand the motivation — is it a warning that you have noticed inaccuracies in the result after using update_weights?

That inaccuracies can occur when using floating point values is one of the basics of Computer Science. Example (which could occur in WeightedIndex::new). How large the error can be is another, much more complex, issue.

As for special treatment for weights which are zero: is your point that total_weight can be trivially reset accurately when all weights are zero? I don't see how this is more useful than periodically replacing the distribution with WeightedIndex::new from calling code.

@arya2
Copy link
Contributor Author

arya2 commented Feb 2, 2023

Is it a warning that you have noticed inaccuracies in the result after using update_weights?

Yep! I should've been clearer about the motivation. It's only meant to help avoid confusion around WeightedError::AllWeightsZero. I made a mistake when reviewing some code and thought it could be helpful to highlight. Perhaps it would be better to update the docs for that error variant instead?

is your point that total_weight can be trivially reset accurately when all weights are zero?

Yeah, it's trivial to do that outside of WeightedIndex if needed so it's probably not very useful.

@dhardy
Copy link
Member

dhardy commented Feb 2, 2023

Okay, I'll go with this.

Ignoring the CI failure on MIPS (unrelated).

@dhardy dhardy merged commit ae4b48e into rust-random:master Feb 2, 2023
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