MAINT Remove all -Woverflow warnings#24919
Closed
jjerphan wants to merge 1 commit intoscikit-learn:mainfrom
Closed
MAINT Remove all -Woverflow warnings#24919jjerphan wants to merge 1 commit intoscikit-learn:mainfrom
jjerphan wants to merge 1 commit intoscikit-learn:mainfrom
Conversation
This removes the -Woverflow warnings observed when building scikit-learn. RAND_R_MAX is the max value for uint8, incrementing it causes an overflow (hence the warning). Elements were originally mentioned but seem to have been left unreviewed, see: scikit-learn#13422 (comment) I think this commit fixes the implementation, yet I comes with a backwards incompatible results and tests for implementation relying on `our_rand_r` fails because results are now different. I see several alternatives to remove the warning while having tests pass - preferred solution: adapt the test suite using the new results so that all tests pass and acknowledge the change of behavior for impacted user-facing APIs in the change-log - accept the quirk of this implementation but hardcode and rename the effective constant - silent the -Woverflow warning by another mean Relates to: scikit-learn#13422 scikit-learn#24895
Member
|
For reference, the Xorshift implementation is shown on Wikipedia:https://en.wikipedia.org/wiki/Xorshift. Additional discussions are made in the Numerical Recipes: http://numerical.recipes/ I still don't get:
|
Member
|
I opened #24955 that fixes the overflow. |
Member
Author
|
Closed as it has been superseded by #24955 which has been merged. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reference Issues/PRs
Towards #24875 for
-WoverflowRelates to #13422 and #24895
What does this implement/fix? Explain your changes.
This removes the -Woverflow warnings observed when building scikit-learn.
RAND_R_MAXis the max value for uint8, incrementing it causes an overflow (hence the warning).I think this commit fixes the implementation, yet I comes with a backwards incompatible results and tests for implementation relying on
our_rand_rfails because results are now different.I see several alternatives to remove the warning while having tests pass:
-Woverflowwarning by another meanAny other comments?
Any other alternative one can think of?
@glemaitre and I, have been capilotracting ourselves on finding the reason of using the modulo as the source linked for the 32bit XorShift generator does not mention.
Any memory @ClemDoum?