BUG: have geometric() raise ValueError on p=0 #11613
Conversation
Currently `np.random.geometric(0) => -9223372036854775808`
There was a problem hiding this comment.
Reviewers with more domain expertise may want to comment on whether this fix is needed & consistent with the RNG NEP--bug fixes still ok in this regard?
If fix is suitable, may also be appropriate to add a Raises section to the docstring?
| if fp < 0.0: | ||
| raise ValueError("p < 0.0") | ||
| if fp <= 0.0: | ||
| raise ValueError("p <= 0.0") |
There was a problem hiding this comment.
It might be suitable to add a unit test for both scalar and array p values of 0 (that failed before and pass now)--ensuring that ValueError is appropriately raised.
|
Sorry, I don't have time to make additional changes here.. IMO this is a
simple fix and ought to be merged as-is; raising an exception is a strict
improvement over the current behavior which is to silently return garbage.
Your call of course!
…On Tue, Jul 24, 2018 at 6:05 PM, Tyler Reddy ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In numpy/random/mtrand/mtrand.pyx
<#11613 (comment)>:
> @@ -4143,15 +4143,15 @@ cdef class RandomState:
if op.shape == ():
fp = PyFloat_AsDouble(p)
- if fp < 0.0:
- raise ValueError("p < 0.0")
+ if fp <= 0.0:
+ raise ValueError("p <= 0.0")
It might be suitable to add a unit test for both scalar and array p
values of 0 (that failed before and pass now)--ensuring that ValueError
is appropriately raised.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#11613 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAsbjqtWwtxaIczohp3a4lhG28nLW75cks5uJ8RbgaJpZM4VdRKo>
.
|
|
In the RNG NEP, I didn't specifically state what would happen between formal acceptance and when the new subsystem lands and the new "don't fix bugs in RandomState distributions" policy will definitely be in effect. I just didn't think about it. I lean towards just turning this into a warning, to more align with the pending policy, but I won't object if people would really like this to strictly raise an error in these situations until the new subsystem lands. |
|
This fix seems like an improvement as
|
|
Thanks @moshelooks |
Sync upstream changes in numpy/numpy#11613, numpy/numpy#11771, and numpy/numpy#12089
Sync upstream changes in numpy#11613, numpy#11771, and numpy#12089
Sync upstream changes in numpy#11613, numpy#11771, and numpy#12089 Update to NumPy 1.12 as the minimum version Fix documentation Add information about Lemire generator Update change log Fix docstring for randint Refactor benchmark with more options Clean code for PEP8 violations Improve performance testing
Sync upstream changes in numpy#11613, numpy#11771, and numpy#12089 Update to NumPy 1.12 as the minimum version Fix documentation Add information about Lemire generator Update change log Fix docstring for randint Refactor benchmark with more options Clean code for PEP8 violations Improve performance testing
Currently
np.random.geometric(0) => -9223372036854775808