-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
Converted output to int64 in sklearn/random_projection.py #19374
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
#DataUmbrella sprint |
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.
@fortune-uwha Could you add a test in sklearn/tests/test_random_projections.py
that would fail on 32bit systems, like in #17111? Let me know if you need help with such a test.
@lorentzenchr I don't really know how to add a test in |
At the bottom of
or for a single test function inside the file
for complete scikit-learn tests suite just Also, have a look at the development guide and pytest. |
I'd suggest a test like test_johnson_lindenstrauss_min_dim():
"""Test Johnson-Lindenstrauss for small eps.
Regression test for #17111: before #19374, 32-bit systems would fail.
"""
assert johnson_lindenstrauss_min_dim(100, eps=1e-5) == 368416070986 |
@lorentzenchr Thank you so much for such detailed feedback! Currently working on it now. Many thanks! |
…-bit systems would fail
…nto random_projection
@lorentzenchr @ogrisel . Successfully added the |
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. Adding tests as a first time contributor, respect!
Thank you for the help @lorentzenchr . Very much appreciated |
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!
@reshamas Thanks for looking into it. |
Reference Issues/PRs
References #17111
What does this implement/fix? Explain your changes.
Converted output to int64 to avoid platform specific overflow. Regression test for #17111: before #19374, 32-bit systems would fail.
Any other comments?
cc: (pair programming partner) @CeeThinwa
#DataUmbrella Sprint