ENH: sparse: Add copy parameter to all .toXXX() methods in sparse matrices

[musically-ut]

This closes #5822.

I have added copy=False as the default arguments for all .to??? conversion methods in sparse matrices. The only places where I left the defaults in place were bsr.py and csr.py's .tobsr method.

All tests pass and no extra functionality has been added, requiring no additional unit-tests.

[codecov-io]

@@            master   #5829   diff @@
======================================
  Files          238     238       
  Stmts        43658   43688    +30
  Branches      8197    8197       
  Methods          0       0       
======================================
+ Hit          34086   34114    +28
  Partial       2602    2602       
- Missed        6970    6972     +2

Review entire Coverage Diff as of a5c0f52

Powered by Codecov. Updated on successful CI builds.

[perimosocordiae]

Looks good to me. I don't like that there are some copy=True defaults, but those will be harder to change without breaking old code.

I'm +1 to merge, but I'll wait a bit to see if anyone else has comments first.

[larsmans]

Some of these methods have no docstrings, and the ones that do promise a copy. We should document that copy=True might cause sharing of data/indices.

To prevent copy-pasting docstrings everywhere, I guess we could...

• rename all these methods to _tocsr etc.
• put the public interfaces with docstrings on the base class, spmatrix, and call the underscore versions from there.

[musically-ut]

@larsmans Unfortunately, not all matrices support all the .toXXX methods, e.g. BSR matrices do not have a .tolil method. I'd rather not throw a run-time error when that happens.

[perimosocordiae]

I'd rather not add an extra layer of method call indirection to all of these methods. Perhaps we can do something like the suggestions in http://stackoverflow.com/q/8100166/10601 ?

[perimosocordiae]

Specifically, we could do something as simple as:

# in coo.py, for example, after the class definition
vars(coo_matrix)['tocsr'].__doc__ = spmatrix.tocsr.__doc__

There's probably a better way to do this, but that's the general idea. I wouldn't be surprised if numpy already has a mechanism for this sort of thing.

[musically-ut]

A cursory look at the documentation guide does not indicate that such a thing is possible or usual.

Also, I may be flawed in my understanding, but if we define .toXXX methods to spmatrix to copy over documentation, then aren't we adding an extra layer of indirection?

In any case, what should be the next steps?

[larsmans]

Also, I may be flawed in my understanding, but if we define .toXXX methods to spmatrix to copy over documentation, then aren't we adding an extra layer of indirection?

We are. I guess @perimosocordiae's solution is simpler. The sparse matrix classes already have some example of __doc__ copying, I think.

[musically-ut]

@larsmans That includes stubbing methods which do not exist across all the matrices (e.g. .tolil() on BSR matrices)? I think throwing a NotImplementedError by default would be the best option in such cases if we are going that route.

Also, indeed, doc copying happens in spmatrix; I have found an example.

[perimosocordiae]

@musically-ut That looks like a good template to follow.

[pv]

The latter tocsr always makes a copy, so it seems to me the call to tocoo can always have copy=False as it did before?

[pv]

Iow, return self.tocoo(copy=False).tocsr(copy=copy).

[musically-ut]

I actually was tempted to change this to:

if copy:
    # One of the following depending on which is more efficient:
    # return self.tocoo(copy=False).tocsr(copy=True)
    return self.tocoo(copy=True).tocsr(copy=False)
else:
    return self.tocoo(copy=False).tocsr(copy=False)

Also, I am not sure I see a reason why copy=False should be the default.
I'll be very surprised if it effects the correctness of the copying. (Does it?)

[perimosocordiae]

What @pv is saying here is that due to the current implementation of coo_matrix.tocsr(), it will always produce a copy no matter what. So doing self.tocoo(copy=False) is always acceptable, so long as you then convert to CSC/CSR format.

He also suggests using the copy kwarg in the final .tocsr() call, because that will protect this method from causing trouble in the future if the CSR/CSC conversion changes to allow non-copying behavior.

The same goes for the BSR->COO->CSC conversion below.

[perimosocordiae]

@musically-ut I'd like to merge this soon, but I think it should include the suggested changes for BSR's tocsc and tocsr methods. Do you have time to make those changes?

[musically-ut]

@perimosocordiae Sorry about the unclean rebase, but could you tell me if the last two commits are in line the kind of changes you had in mind?

If so, I can base it off master, create a new branch and make another PR.

~
ut

PS: I was seeing an odd test failure with lgmres on my local machine after merging master back in.

[perimosocordiae]

Yeah, the last two commits look pretty good, minus some small nitpicks. It should be possible to clean up the history in the existing branch and force push, but you're free to close this and make a new PR if that's easier on your end.

[musically-ut]

Great! I have fixed the typos (except one indicies in csr.h), squashed the two into a single commit and redone the rebase. This should be ready to merge.

PS: There was one other marginally unrelated test failure on my local computer in test_basic.TestLstsq after the rebase.

[perimosocordiae]

This looks like a bug: one of these should have copy=copy

[musically-ut]

I've taken care of the issues raised. test_sparse_format_conversions actually tests out all the conversions, so there isn't need for more tests. Otherwise, I would like some ideas on how should this code be tested in isolation.

Also, should I work on including the benchmarks as a part of this PR itself?

Note that I am planning on having a more comprehensive set of benchmarks at musically-ut/scipy-sparse-benchmarks anyway.

[perimosocordiae]

Thanks @musically-ut, merged. We can add benchmarks in a separate PR.