Skip to content

Add note about floating point weights in update_weights docs#1280

Merged
dhardy merged 4 commits into
rust-random:masterfrom
arya2:fix-update-weights
Feb 2, 2023
Merged

Add note about floating point weights in update_weights docs#1280
dhardy merged 4 commits into
rust-random:masterfrom
arya2:fix-update-weights

Conversation

@arya2

@arya2 arya2 commented Jan 26, 2023

Copy link
Copy Markdown
Contributor

Motivation

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

@dhardy

dhardy commented Jan 31, 2023

Copy link
Copy Markdown
Member

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

arya2 commented Feb 2, 2023

Copy link
Copy Markdown
Contributor Author

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.

Comment thread src/distributions/weighted_index.rs Outdated
Comment thread src/distributions/weighted_index.rs Outdated
@dhardy

dhardy commented Feb 2, 2023

Copy link
Copy Markdown
Member

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