Bug in the implementation of sample for Finite Discrete RVs

13 views
Skip to first unread message

kaddy

unread,
Nov 8, 2025, 2:31:37 AM (yesterday) Nov 8
to sympy
I'm creating this thread to discuss the change suggested in this PR. This PR does fix a serious bug, at least partially which wouldn't allow us to sample discrete finite RVs (even the Coin RV of SymPy with default constructor parameters!). The details of the fix are highlighted clearly in the PR description, but we can discuss it further over here. I believe this implementation is superior to the current implementation because it respects the object types of the sample space and doesn't force them to be Python native types int and float. This is particularly useful if you need to preserve precision or use symbolic computation in downstream processing (e.g. not being forced to convert rationals or irrationals in the sample space to integers for sampling). 

However, this leads to a test failing (sympy/stats/tests/test_rv.py:211). The test fails because it does a naive isinstance(sample(X+Y), float) check on the sampling output of a random variable defined as the sum of a standard normal and a Die RV. As discussed above, the output, if implemented correctly of sampling a Die RV should be a SymPy Integer, thus the type of sample(X+Y) will be a SymPy float. Since SymPy float is not a derived class of Python's native float, this test fails. 

As a solution, I propose increasing the scope of this test to check for any valid floating point type or if we're being pedantic, the SymPy float type. We cannot have a correct implementation of sample for all the test cases mentioned in the PR (and several more, complicated ones as well!) and this test case. 

Would love to know the maintainers' thoughts on the same. 

P.S. The PR is an incomplete draft PR for now. Further commits handling cases when the library is not specified as SciPy and regression tests etc will be added soon after I get some feedback or discussion on the approach.  
Reply all
Reply to author
Forward
0 new messages