Skip to content
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

[REVIEW] Fix Attribute error on IPCA #3183 and PCA input type #3190

Merged
merged 4 commits into from
Dec 1, 2020

Conversation

lowener
Copy link
Contributor

@lowener lowener commented Nov 25, 2020

This PR is fixing the attribute error of #3183, and additional bugs on the input type of PCA (sparse_scipy_to_cp() function call missed an argument) and on the shape of self.singular_values_.

I am also adding additional tests on the bug fixed here.

@lowener lowener requested a review from a team as a code owner November 25, 2020 17:11
@GPUtester
Copy link
Contributor

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@codecov-io
Copy link

codecov-io commented Nov 25, 2020

Codecov Report

Merging #3190 (49097ff) into branch-0.17 (f1abcb8) will increase coverage by 0.02%.
The diff coverage is 88.88%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.17    #3190      +/-   ##
===============================================
+ Coverage        70.99%   71.02%   +0.02%     
===============================================
  Files              198      198              
  Lines            15729    15731       +2     
===============================================
+ Hits             11167    11173       +6     
+ Misses            4562     4558       -4     
Impacted Files Coverage Δ
python/cuml/decomposition/pca.pyx 93.70% <87.50%> (+1.51%) ⬆️
...cuml/experimental/decomposition/incremental_pca.py 86.75% <100.00%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1abcb8...1704b02. Read the comment docs.

@lowener lowener changed the title [WIP] Fix Attribute error on ICPA #3183 and PCA input type [REVIEW] Fix Attribute error on ICPA #3183 and PCA input type Nov 26, 2020
@cjnolet cjnolet added 3 - Ready for Review Ready for review by team Cython / Python Cython or Python issue labels Nov 30, 2020
Copy link
Member

@divyegala divyegala left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Mickael, and congratulations on your first PR! I distinctly remember there being tests for whiten path when I wrote them. I think recent CumlArray changes and internal effort to speed up tests changed both these things at the same time.


if return_sparse:
pytest.skip("Loss of information in converting to cupy sparse csr")

X = cupyx.scipy.sparse.random(nrows, ncols, density=0.07, dtype=cp.float32,
random_state=10)
if not(cupy_input):
Copy link
Contributor

Choose a reason for hiding this comment

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

for future reference - you don't need these parentheses

@JohnZed JohnZed changed the title [REVIEW] Fix Attribute error on ICPA #3183 and PCA input type [REVIEW] Fix Attribute error on ICPA #3183 and PCA input type [skip-ci] Dec 1, 2020
@JohnZed JohnZed changed the title [REVIEW] Fix Attribute error on ICPA #3183 and PCA input type [skip-ci] [REVIEW] Fix Attribute error on ICPA #3183 and PCA input type Dec 1, 2020
@JohnZed JohnZed added 6 - Okay to Auto-Merge bug Something isn't working non-breaking Non-breaking change labels Dec 1, 2020
@rapids-bot rapids-bot bot merged commit 4b2aaae into rapidsai:branch-0.17 Dec 1, 2020
@lowener lowener deleted the 017_fix_ipca branch December 1, 2020 16:06
@lowener lowener changed the title [REVIEW] Fix Attribute error on ICPA #3183 and PCA input type [REVIEW] Fix Attribute error on IPCA #3183 and PCA input type Jan 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team bug Something isn't working Cython / Python Cython or Python issue non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants