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

Trap weighted index overflow #1353

Merged
merged 4 commits into from
Dec 30, 2023
Merged

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Oct 31, 2023

Closes #1309. This approach is likely sub-optimal but without significant overhead (not benchmarked), and only to a distribution constructor.

@dhardy dhardy force-pushed the weighted_index_overflow branch from 39b0c17 to 4deeff2 Compare December 4, 2023 08:00
@dhardy
Copy link
Member Author

dhardy commented Dec 4, 2023

So cargo test fails: Rust detects an overflow in total_weight += w.borrow(); on debug builds.

We want checked_add here, but (like many things) there's no trait for that in std. (There is in num-traits.)

Alternative: use wrapping_add to disable the built-in overflow check, then manually check. But again, no trait. Instead, we can use core::num::Wrapping... but the method needs to support f32 which Wrapping does not.

So I think it comes down to:

  1. Depend on num-traits in rand (currently only rand_distr depends on this)
  2. Or add our own trait (already used in other places, e.g. Float).

I opted for the second choice here:

/// Bounds on a weight
///
/// See usage in [`WeightedIndex`].
pub trait Weight: Clone {
    /// Representation of 0
    const ZERO: Self;

    /// Checked addition
    ///
    /// -   `Result::Ok`: On success, `v` is added to `self`
    /// -   `Result::Err`: Returns an error when `Self` cannot represent the
    ///     result of `self + v` (i.e. overflow). The value of `self` should be
    ///     discarded.
    fn checked_add_assign(&mut self, v: &Self) -> Result<(), ()>;
}

@dhardy
Copy link
Member Author

dhardy commented Dec 4, 2023

What this omits: the impls of checked_add_assign for float types always succeed since floats "overload to INF representation". It may be better for our use-case to error here.

Also: the signature of checked_add_assign is tailored to our use-case and differs from e.g. num_traits::ops::checked::CheckedAdd.

@vks
Copy link
Collaborator

vks commented Dec 14, 2023

I really like the simplified trait bounds!

My only concern is that Weight is public: Almost any change to it will be a breaking change.

Copy link
Member Author

@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.

Yes, the bounds now look much nicer!

Comment on lines +249 to +255
/// Checked addition
///
/// - `Result::Ok`: On success, `v` is added to `self`
/// - `Result::Err`: Returns an error when `Self` cannot represent the
/// result of `self + v` (i.e. overflow). The value of `self` should be
/// discarded.
fn checked_add_assign(&mut self, v: &Self) -> Result<(), ()>;
Copy link
Member Author

Choose a reason for hiding this comment

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

The only thing that concerns me slightly is that this is a fairly complex signature that, as you say, we can't easily change later.

Possibly the standard checked_add signature would be better, but this one is closer to what we actually need (considering we don't have a Copy bound).

@dhardy dhardy merged commit e9a27a8 into rust-random:master Dec 30, 2023
12 checks passed
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.

WeightedIndex produces unexpected results when sum of weights does not fit in the integer type
2 participants