BUG: Fix bounds checking for random.logseries#22450
Conversation
|
I agree that p=1 should not be allowed, but the current behavior with p=0 looks correct to me. The limit of the distribution is well-defined for p → 0: it is the trivial distribution with support {1}. And it looks like the code currently handles that case correctly. I think it would be OK to continue handling this edge case. |
Logseries previously did not enforce bounds to be strictly exclusive for the upper bound, where it leads to incorrect behavior. The NOT_NAN check is removed, since it was never used: The current bounded version always excludes NaNs.
a9aacda to
8750d04
Compare
|
Yeah, it does. I was unsure about that, since technically the distribution is not defined. But with float inaccuracies, I guess it is more pragmatic to just allow it in either case. Anyway changed, will not have an array fast-path function now, but I suspect that this is OK for |
WarrenWeckesser
left a comment
There was a problem hiding this comment.
Looks good to me, let's see if @bashtage has any suggestions.
bashtage
left a comment
There was a problem hiding this comment.
I think the mtrand changes are outside of the allowed scope per NEP19.
| ---------- | ||
| p : float or array_like of floats | ||
| Shape parameter for the distribution. Must be in the range (0, 1). | ||
| Shape parameter for the distribution. Must be in the range [0, 1). |
There was a problem hiding this comment.
Not 100% sure that this change is allowed. I think this would be either an enhancement or a bug fix, and IIRC the NEP says that these should not be applied to mtrand.
There was a problem hiding this comment.
Yes, that seems like a reasonable take.
There was a problem hiding this comment.
This is only a doc update? There is no change in behavior. 0 always worked, as pointed out by Warren, 1 never worked it is just explicit now.
|
Does the generator behave sensibly in a neighborhood of 0+? Or is 0 a special case and other tiny numbers are less reliable? |
|
I have not spelled it out, but the path for 1 hangs, so 1 was never a valid input. Changing 0 to be excluded would indeed be a change that may be covered by the NEP. |
|
Thanks Sebastian. |
Logseries previously did not enforce bounds to be strictly exclusive. This leads to incorrect behavior for 0 and 1. Add a new path to implement the correct bounds-checking.
The NOT_NAN check is removed, since it was never used: The current bounded version always excludes NaNs.