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

Sparse matrix inequalities #2586

Merged
merged 8 commits into from Jun 27, 2013

Conversation

Projects
None yet
3 participants
@cowlicks
Copy link
Contributor

cowlicks commented Jun 20, 2013

Sparse matrices can now use the <, >, <=, and >= operators, with scalars, dense matrices (with broadcasting), and sparse matrices (of the same shape). It should be noted that when comparing with dense matrices the sparse matrix should be on the left hand side.

@cowlicks

This comment has been minimized.

Copy link
Contributor

cowlicks commented Jun 20, 2013

Ah, I implemented complex inequalities based on the Euclidian norm, NumPy does something else. Fixing.

@pv

This comment has been minimized.

Copy link
Member

pv commented Jun 20, 2013

Yes, Numpy treats complex number comparisons in lexical order.

@cowlicks

This comment has been minimized.

Copy link
Contributor

cowlicks commented Jun 20, 2013

A lot of these tests throw warnings, which is intended, but they show up when running the tests which is undesirable. I need to change these warnings to SparseEifficiencyWarnings so they will be ignored.

@rgommers

This comment has been minimized.

Copy link
Member

rgommers commented Jun 20, 2013

Pick the warning type based on what makes most sense. If then warnings still show up in the test output, use in the test:

with warnings.catch_warnings():
    warnings.filterwarnings(...)
    test_code_here
@cowlicks

This comment has been minimized.

Copy link
Contributor

cowlicks commented Jun 20, 2013

@rgommers I think SparseEfficiencyWarning makes sense here.

@pv

View changes

scipy/sparse/sparsetools/complex_ops.h Outdated
}
bool operator<=(const c_type& B) const{
if (npy_type::real == B) {
return npy_type::imag < c_type(0);

This comment has been minimized.

@pv

pv Jun 24, 2013

Member

Should be <=

Not caught by tests? This may be an unused code path, so I don't know how to test it...

This comment has been minimized.

@cowlicks

cowlicks Jun 24, 2013

Contributor

Oops yes.

If I understand this correctly, this code path is called when some complex_wrapper var is compared with a c_type var using <=. So when const c_type &B is declared, what is c_type(0)?

@pv

This comment has been minimized.

Copy link
Member

pv commented Jun 24, 2013

Looks good, apart from the one < vs <= issue.

@cowlicks

This comment has been minimized.

Copy link
Contributor

cowlicks commented Jun 24, 2013

I should note, having the non strict inequalities <= and >= adds 24,990 lines of code to csr_wrap.cxx, csc_wrap.cxx, and bsr_wrap.cxx.

But the non strict inequalities are generally inefficient and, in every case I can think of, be replaced by strict inequalities and ==. Are these worth the extra size? I have not been able to reliably check the difference in compile time but I can do that later.

@pv

View changes

scipy/sparse/compressed.py Outdated
raise NotImplementedError(" >= and <= don't work with 0.")
elif 0 <= other:
warn("Comparing a sparse matrix with a scalar less than zero "
"using <= is inneficient, try using < instead.", SparseEfficiencyWarning)

This comment has been minimized.

@pv

pv Jun 24, 2013

Member

Typo: inefficient

It's repeated in the other messages, too.

# sparse/sparse
assert_array_equal(dat <= dat2, (datsp <= datsp2).todense())
assert_array_equal(datcomplex <= dat2, (datspcomplex <= datsp2).todense())
# mix sparse types

This comment has been minimized.

@pv

pv Jun 24, 2013

Member

Here, should also check the right-hand versions of the operations, dat2 <= dat,
ditto for the other ops.

(Nitpick: usually the syntax is assert_equal(result, expected), here it's assert_equal(expected, result))

@pv

This comment has been minimized.

Copy link
Member

pv commented Jun 24, 2013

I wouldn't worry about such relatively small code size increases.

Here it seems the _ge_, _le_ operations speed up one scalar comparison case, so they're of some use. The implementation can perhaps be refactored when the time comes for refactoring the sparse matrices for broadcasting.

@cowlicks

This comment has been minimized.

Copy link
Contributor

cowlicks commented Jun 25, 2013

Closing while I rebase this.

@cowlicks cowlicks closed this Jun 25, 2013

@cowlicks

This comment has been minimized.

Copy link
Contributor

cowlicks commented Jun 25, 2013

Rebased.

Edit: Looks like I did something wrong... Why are all these other commits, which are already in master, in this pull request?

@cowlicks cowlicks reopened this Jun 25, 2013

@pv

This comment has been minimized.

Copy link
Member

pv commented Jun 25, 2013

The rebase looks OK to me in git, maybe the strangeness is just github thing. The changes look OK too.

However, this idiom

x = ones(foo) * other

would be better written as

x = empty(foo)
x.fill(other)

Btw, other stuff on the TODO list: npy_bool_wrapper is still defined as int8, so things like this function wrong:

>>> x = csr_matrix([[True,False,True]])
>>> x = x + x; print(x.todense())
[[ True False  True]]
>>> x = x + x; print(x.todense())
[[ True False  True]]
>>> x = x + x; print(x.todense())
[[ True False  True]]
>>> x = x + x; print(x.todense())
[[ True False  True]]
>>> x = x + x; print(x.todense())
[[ True False  True]]
>>> x = x + x; print(x.todense())
[[ True False  True]]
>>> x = x + x; print(x.todense())
[[ True False  True]]
>>> x = x + x; print(x.todense())  # <--- !
[[False False False]]

Needs to be fixed + tests added.

@cowlicks

This comment has been minimized.

Copy link
Contributor

cowlicks commented Jun 27, 2013

I have a branch with a corrected. bool_ops.h, I'll put it in another PR because I think it needs a lot of review.

pv added a commit that referenced this pull request Jun 27, 2013

Merge pull request #2586 from cowlicks/inequalities-override
ENH: sparse: implement inequality ops for sparse matrices

@pv pv merged commit 0e1dd62 into scipy:master Jun 27, 2013

1 check passed

default The Travis CI build passed
Details
@pv

This comment has been minimized.

Copy link
Member

pv commented Jun 27, 2013

Thanks, I think this is done.

@cowlicks cowlicks deleted the cowlicks:inequalities-override branch Jun 27, 2013

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