Add extra tests for random.binomialvariate#112325
Conversation
|
Is the solving a real problem that you've had or is this just a theoretical issue? In general, the module shies away from such checks unless we find that they are really necessary. For example, |
|
I happened to check the coverage of I agree that the function should be short and fast, but should we keep the same standard? For example, the check for In However, I understand if you don't want to add the extra |
I think that would be best. |
Could you share your thoughts about |
|
#81620 has been closed a long time ago. Could you please open a new issue for discussing the feasibility of such changes? |
It's a bit of a personal preference. This module tends to use the former such as the |
|
@serhiy-storchaka I don't think we need to open another issue for a minor code edit. I'll just remove the issue reference from this PR. |
random.binomialvariaterandom.binomialvariate
I'm actually perfectly fine with truthy value check for sequences. I only have hesitations about numerical values. It is faster than Anyway, I removed the code change in |
|
Thanks for the extra tests and for the polite discussion. |
Currently
random.binomialvariatedoes not check the type ofnwhile the docs clearly states it should be an integer (and mathmatically it should be an integer). This could cause things likerandom.binomialvariate(3.5, 1) == 3.5. In this PR, a check for the type ofnis added.I also added the check for
n == 0. This introduces an extra if check on the common path.n == 0works without this check, it just requires some extra calculation. I can take it out if we want to make the common path slightly faster.if not cis changed toif c == 0, which I believe is much better when checking a number against0.A couple of test cases are added as well, notably
self.assertEqual(B(5, 1e-8), 0), which will trigger the uncovered path in BG method underif c == 0.