Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Merged
merged 5 commits into from
Feb 12, 2021

Conversation

fortune-uwha
Copy link
Contributor

@fortune-uwha fortune-uwha commented Feb 6, 2021

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

@reshamas
Copy link
Member

reshamas commented Feb 6, 2021

#DataUmbrella sprint

Copy link
Member

@lorentzenchr lorentzenchr left a 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.

@fortune-uwha
Copy link
Contributor Author

@lorentzenchr I don't really know how to add a test in sklearn/tests/test_random_projections.py. But I'll appreciate any help given to do so.

@lorentzenchr
Copy link
Member

lorentzenchr commented Feb 6, 2021

At the bottom of sklearn/tests/test_random_projections.py you add another function, the name has to start with test_. Usually you assert a know result. If you compare numpy arrays and are fine with very tiny deviations, you use assert_allclose(arr1, arr2). Have a look in the file itself for actual tests.
You can invoke the tests of a file or directory by calling it with pytest (which you have to install, e.g. pip install pytest):

pytest sklearn/tests/test_random_projection.py

or for a single test function inside the file

pytest sklearn/tests/test_random_projection.py::test_works_with_sparse_data

for complete scikit-learn tests suite just pytest inside the scikit-learn directory (beware: this takes a lot of time!).

Also, have a look at the development guide and pytest.

@lorentzenchr
Copy link
Member

lorentzenchr commented Feb 6, 2021

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

@fortune-uwha
Copy link
Contributor Author

@lorentzenchr Thank you so much for such detailed feedback! Currently working on it now. Many thanks!

@fortune-uwha
Copy link
Contributor Author

@lorentzenchr @ogrisel . Successfully added the sklearn/tests/test_random_projections.py test. Is this ok please?

Copy link
Member

@lorentzenchr lorentzenchr left a 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!

@fortune-uwha
Copy link
Contributor Author

Thank you for the help @lorentzenchr . Very much appreciated

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@rth rth merged commit abd1597 into scikit-learn:main Feb 12, 2021
@reshamas
Copy link
Member

reshamas commented Feb 18, 2021

@rth @cmarmo
Does this merged PR close issue #17111 ?

@lorentzenchr
Copy link
Member

@reshamas Thanks for looking into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.