8000 [MRG+1] Scaling a sparse matrix along axis 0 should accept a csc by default by MechCoder  8000 · Pull Request #5791 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

Comments

[MRG+1] Scaling a sparse matrix along axis 0 should accept a csc by default#5791

Merged
GaelVaroquaux merged 1 commit intoscikit-learn:masterfrom
MechCoder:scale_csc
Nov 13, 2015
Merged

[MRG+1] Scaling a sparse matrix along axis 0 should accept a csc by default#5791
GaelVaroquaux merged 1 commit intoscikit-learn:masterfrom
MechCoder:scale_csc

Conversation

@MechCoder
Copy link
Member
@MechCoder MechCoder commented Nov 11, 2015

For scaling a csc_matrix along axis 0, one pass is made across n_features.

For scaling a csc_matrix along axis 0, two passes are made across non_zero_values and a pass across n_features.

So the csc_matrix should be accepted by default. This can be validated by this quick benchmark.

X = np.random.rand(10000, 10000)
X[X < 0.8] = 0.0
csr = sparse.csr_matrix(X)
csc = sparse.csc_matrix(X)
%timeit csr_mean_variance_axis0(csr)
10 loops, best of 3: 83.7 ms per loop
%timeit csc_mean_variance_axis0(csc)
10 loops, best of 3: 41.2 ms per loop

@MechCoder MechCoder mentioned this pull request Nov 11, 2015
Closed
@MechCoder
Copy link
Member Author
MechCoder commented Nov 11, 2015

@giorgiop ?

@giorgiop
Copy link
Contributor
giorgiop commented Nov 12, 2015

I think this makes sense, and I agree to remove the superfluous copies as well. Could you time test_data and see how much we gain in runtime in there, for example?

@MechCoder
Copy link
Member Author
MechCoder commented Nov 12, 2015

I don't think this would matter much in the unless done in a loop. But it is good to have for correctness.

I do not give similar speeds between this branch and master.

@MechCoder
Copy link
Member Author
MechCoder commented Nov 12, 2015

Well, I see only two places where a sparse matrix is used in test_data.py. So it definitely shouldn't matter in that.

@giorgiop
Copy link
Contributor
giorgiop commented Nov 12, 2015

Well, I see only two places where a sparse matrix is used in test_data.py. So it definitely shouldn't matter in that.

You have also removed a copy in the dense case. But, yeah, probably not worth a try as inputs in tests are tiny anyway.

@MechCoder
Copy link
Member Author
MechCoder commented Nov 12, 2015

Do you have any more comments or can I update the PR description to +1?

@giorgiop
Copy link
Contributor
giorgiop commented Nov 13, 2015

+1 from my side!

@GaelVaroquaux GaelVaroquaux changed the title [MRG] Scaling a sparse matrix along axis 0 should accept a csc by default [MRG+1] Scaling a sparse matrix along axis 0 should accept a csc by default Nov 13, 2015
@GaelVaroquaux
Copy link
Member
GaelVaroquaux commented Nov 13, 2015

LGTM. Thank you. Merging.

GaelVaroquaux added a commit that referenced this pull request Nov 13, 2015
@GaelVaroquaux
Merge pull request #5791 from MechCoder/scale_csc
d644e8e 
[MRG+1] Scaling a sparse matrix along axis 0 should accept a csc by default
@GaelVaroquaux GaelVaroquaux merged commit d644e8e into scikit-learn:master Nov 13, 2015
@MechCoder MechCoder deleted the scale_csc branch November 13, 2015 16:10
@MechCoder
Copy link
Member Author
MechCoder commented Nov 13, 2015

forgot to update whatsnew. should I?

amueller
amueller reviewed Nov 14, 2015
View reviewed changes
sklearn/preprocessing/data.py
Copy link
Member
@amueller amueller Nov 14, 2015

Choose a reason for hiding this comment

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

Why not ['csc, 'csr']?

Copy link
Member Author
@MechCoder MechCoder Nov 14, 2015

Choose a reason for hiding this comment

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

I did not write the initial code, but I think the idea is to convert it into a format where the column operations are sufficiently faster. I just maintained that convention because converting it to csr would be non-intuitive.

Copy link
Member Author
@MechCoder MechCoder Nov 14, 2015

Choose a reason for hiding this comment

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

Are you afraid that this will not be backward compatible?

Copy link
Member
@amueller amueller Sep 12, 2016

Choose a reason for hiding this comment

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

How much faster is converting to csc and multipying compared to not converting? ['csc', 'csr'] will not convert to CSR but will let CSR pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@MechCoder @giorgiop @GaelVaroquaux @amueller
0