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

ENH: allow addition/substraction of 0 to a sparse matrix #171

Merged
merged 3 commits into from Mar 12, 2012

Conversation

dlax
Copy link
Member

@dlax dlax commented Feb 29, 2012

No description provided.

@jakevdp
Copy link
Member

jakevdp commented Feb 29, 2012

I'd change the error text to specify "adding a nonzero scalar..."

@jakevdp
Copy link
Member

jakevdp commented Feb 29, 2012

Also, this behavior may lead to some confusion: with dense matrices, adding zero returns a copy. Sparse matrices should probably behave the same way to be consistent.

@dlax
Copy link
Member Author

dlax commented Feb 29, 2012

Jacob Vanderplas wrote:

I'd change the error text to specify "adding a nonzero scalar..."

Also, this behavior may lead to some confusion: with dense matrices, adding zero returns a copy. Sparse matrices should probably behave the same way to be consistent.

Done.

@jakevdp
Copy link
Member

jakevdp commented Feb 29, 2012

Out of curiosity, what use case do you have in mind for this? I can't think of any case where I'd want to add a scalar to a matrix, raising an error unless the scalar is zero. That would essentially require me to explicitly check to see if the scalar is zero before the operation. But if I know it's zero, why would I do this operation, except as a roundabout path to a matrix copy?

@dlax
Copy link
Member Author

dlax commented Feb 29, 2012

Jacob Vanderplas wrote:

Out of curiosity, what use case do you have in mind for this?

sum(a_list_of_sparse_matrices)

@jakevdp
Copy link
Member

jakevdp commented Mar 1, 2012

Makes sense!

@dlax
Copy link
Member Author

dlax commented Mar 12, 2012

I'd like to merge this soon unless somebody objects.

@jakevdp
Copy link
Member

jakevdp commented Mar 12, 2012

Sorry - I lost track of this in all the buzz around pycon!
Before merge, we'll need a unit test for this feature.

@dlax
Copy link
Member Author

dlax commented Mar 12, 2012

Jacob Vanderplas wrote:

Before merge, we'll need a unit test for this feature.

Test added.

By the way, there's a test in test_construct/test_diags_bad that
appears to fail in this branch but not in master. I'm not sure why this
happens but this may be related the merge of #167.

@jakevdp
Copy link
Member

jakevdp commented Mar 12, 2012

Nice work on the test, I'll merge this now.
If you could write the scipy-dev list with the traceback for the test_diags_bad error, that would be very helpful.

jakevdp added a commit that referenced this pull request Mar 12, 2012
ENH: allow addition/substraction of 0 to a sparse matrix
@jakevdp jakevdp merged commit c606186 into scipy:master Mar 12, 2012
@dlax
Copy link
Member Author

dlax commented Mar 12, 2012

Jacob Vanderplas wrote:

If you could write the scipy-dev list with the traceback for the test_diags_bad error, that would be very helpful.

The test failure was only in the topic branch. Now that you've merged
into master, there's no more failure.

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.

None yet

2 participants