test cases and NEWS for #30372 (sparse matmul) #30433
Conversation
|
I would also mention the performance improvements for sparse matmul in the NEWS. |
|
Should we also add Can we reduce the test cases to smaller matrices (not larger than 100x100) have just two: something with less than 1 nonzero per row/col on avg, and something that is several nonzeros per row/col. Too many tests here increases the testing time for everyone, and we need to be sensitive to that. |
ViralBShah
changed the title
I think, the |
NEWS.md
Outdated
| #### SparseArrays | ||
|
|
||
| * `spmatmul` no longer has the obsolete optional keyword argument `sortindices` ([#30372]). | ||
| * performance improvements for `spmatmul` and hence for sparse matrix-matrix multiplication. |
There was a problem hiding this comment.
AFAICT we usually don't include things like this in news.
There was a problem hiding this comment.
Is a line like this ok or should I leave it out completely?
* performance improvements for sparse matrix-matrix multiplication ([#30372]).
There was a problem hiding this comment.
@fredrikekre Why do we not mention performance improvements in NEWS? That generally sounds like users would appreciate it.
There was a problem hiding this comment.
I'm just saying that we have not done it before.
There was a problem hiding this comment.
Performance improvements can make the difference between usable and unusable in certain application scenarios. What do you recommend?
There was a problem hiding this comment.
@fredrikekre Fair enough. I think it is a good idea and I will merge it now. I also think that performance improvements are generally good to include across the board. If it is something we decide that we shouldn't do, we can always remove it.
|
Do we add NEWS entries to the 1.0.x releases? |
StefanKarpinski
added
backport 1.1
triage
#30372 needs some extra test cases to verify the implementation changes.