BUG+TST: setdiag implementation for dia_matrix (Close #2931) + test_setdiag (was untested) #2932
Conversation
…est_setdiag (was untested) Revealed similar lack of setdiag imlementation for COO and BSR matrices (TODO)
|
Before this PR can be merged, either the BSR/COO setdiags need to be implemented, or the tests stubbed out to knownfailures (see the bottom of TestDOK class definition on how to do that) |
|
here you go (marked as known failure) |
| M, N = self.shape | ||
| if (not k in self.offsets): | ||
| raise ValueError( | ||
| "dia matrix does not support assignment for unknown diagonals. " |
There was a problem hiding this comment.
It might be nice to handle also this case, as it seems it could be done with a couple of lines of code.
There was a problem hiding this comment.
pull requests are welcome!
There was a problem hiding this comment.
Sure, I'm not saying here that you should tackle it, but that it should be addressed before merging this.
There was a problem hiding this comment.
and meanwhile scipy should stay instead with a completely broken version instead of a partially implemented with a consistent error message (may be actually NotImplemented instead of ValueError would be better)?
oh well
There was a problem hiding this comment.
Yes. The next release will be in next spring. It's better to do it right in the first round.
|
Bump. I've fixed the outstanding TODOs for this PR here, but @yarikoptic seems to be busy with other things. I can roll all the changes into a new PR on this repo, if that would help. |
enable setdiag for non pre-existing offsets
|
Thank you @perimosocordiae and sorry -- I have missed your PR in time. Merged/updated this PR now |
|
Anyone from scipy team could possibly have a look and direct us further to get this PR finally accepted? |
|
@pv your comments seem to have been addressed. Do you have time to review this, or should I have a closer look? |
BUG: sparse: setdiag implementation for dia_matrix
|
Thanks, rebased and merged. |
Revealed similar lack of setdiag imlementation for COO and BSR matrices (TODO)