-
-
Notifications
You must be signed in to change notification settings - Fork 26k
FIX Param validation Interval error for large integers #26648
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR @naoise-h !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @naoise-h. This is the appropriate fix imo.
out of curiosity, what is the parameter that you want to set to such a big int ? |
Linting PassedAll linting checks passed. Your pull request is in excellent shape! ☀️ |
I was trying to seed a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as well. @jeremiedbb why is this marked "no changelog needed"? Wasn't this bug already there in 1.2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not wait for the refactoring of utils
to merge this one. Thanks @naoise-h.
…26648) Co-authored-by: jeremie du boisberranger <jeremiedbb@yahoo.fr>
…26648) Co-authored-by: jeremie du boisberranger <jeremiedbb@yahoo.fr>
At present, param validation on
Interval
returns a TypeError when provided with a large integer (greater than 64 bits). This is because of thenp.isnan
call, which attempts to convert to a Numpy integer, of which no type exists greater than 64 bits. This issue came to light when trying to usenumpy.SeedSequence().entropy
(a 128-bit integer) as arandom_state
seed.What does this implement/fix? Explain your changes.
A TypeError is raised unexpectedly:
Instead of calling
np.isnan()
on the value alone, it now first checks if the value is an integer (since integers cannot benan
). Tests have been added to check it works with large integers (including integers larger than those representible by a float, approx2**1024
and larger).After these changes, we have the following output, as expected:
Any other comments?
math.isnan
instead, which raises anOverflowError
when conversion to float fails._NanConstraint
will also throw anOverflowError
if provided with a large integer (> 2**1024), but its only use is withinMissingValues
which already has an Integral class check