Potentially undefined behaviour when initialised with invalid probability distribution. #7

Activity

1. asmith26 commented on Nov 11, 2019

   

   asmith26

   on Nov 11, 2019

   Owner

   More actions

   Hi @s-ilyas,

   Good catch, you are correct that sum(dist) == 1.

   Would you like to a submit a PR request?
   Many thanks for your contribution!
2. s-ilyas commented on Nov 11, 2019

   

   s-ilyas

   on Nov 11, 2019

   Author

   More actions

   Sure, will do sometime today.

   👍React with 👍1Reacted by asmith26
3. austin-bowen commented on Feb 13, 2023

   

   austin-bowen

   on Feb 13, 2023

   Contributor

   More actions

   FWIW, in some of my own code I have subclassed VoseAlias and overrode __init__ to perform this check like so:

   import vose_sampler
   from math import isclose
   
   class MyVoseAlias(vose_sampler.VoseAlias):
       def __init__(self, dist, rng=None, prob_sum_tol: float = 1e-9):
           prob_sum = sum(dist.values())
           if not isclose(prob_sum, 1., rel_tol=0, abs_tol=prob_sum_tol):
               raise ValueError(f'Probabilities did not add up to 1.0 (+/- {prob_sum_tol}). prob_sum={prob_sum}')
   
           super().__init__(dist, rng)

   In my testing, I didn't start getting the ValueError thrown until I put about 100 million distinct items -- each with probability 1 / 100M -- into the VoseAlias.

   I would be a little wary of putting that code right into VoseAlias without a major version bump, as it could be a breaking change for some people who might be relying (knowingly or otherwise) on the lack of probability sum checking.

   If you do put this into VoseAlias, I'd probably replace the raise ValueError with warnings.warn so it doesn't break anyone's existing code.

   👍React with 👍1Reacted by asmith26
4. austin-bowen commented on Feb 13, 2023

   

   austin-bowen

   on Feb 13, 2023

   Contributor

   More actions

   Might also want to check that all probabilities are between 0 and 1, cuz you could still get around the sum check with something insane like {'H': 1.1, 'T': -0.1} 😂

   👍React with 👍1Reacted by asmith26
5. austin-bowen commented on Feb 13, 2023

   

   austin-bowen

   on Feb 13, 2023

   Contributor

   More actions

   Something like:

   if any(p < 0 or p > 1 for p in dist.values()):
       raise ValueError(f'All probabilities must be in the range [0, 1].')

   👍React with 👍1Reacted by asmith26
6. webbrain-one commented on Aug 12, 2026

   

   webbrain-one

   on Aug 12, 2026

   More actions

   Thanks for flagging this! I’ve opened a PR to address it: #16. It adds a quick validation check to ensure the provided probability distribution sums to 1, which should prevent the undefined behavior you noticed. Let me know if you have any feedback or if the description needs any additional clarification. Appreciate the catch!

   👍React with 👍1Reacted by asmith26