BUG: sparse: Fix LIL full-matrix assignment #18211
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
perimosocordiae
added
scipy.sparse
defect
tylerjereddy
added
the
backport-candidate
scipy/sparse/_lil.py
Outdated
| # Fast path for full-matrix sparse assignment. | ||
| if (isinstance(row, slice) and isinstance(col, slice) and | ||
| row == slice(None) and col == slice(None) and | ||
| isspmatrix(x)): |
There was a problem hiding this comment.
In case it helps, this patch seems to allow the full suite to pass for me locally:
--- a/scipy/sparse/_lil.py
+++ b/scipy/sparse/_lil.py
@@ -322,7 +322,7 @@ class lil_matrix(spmatrix, IndexMixin):
# Fast path for full-matrix sparse assignment.
if (isinstance(row, slice) and isinstance(col, slice) and
row == slice(None) and col == slice(None) and
- isspmatrix(x)):
+ isspmatrix(x) and self.shape == x.shape):
x = self._lil_container(x, dtype=self.dtype)
self.rows = x.rows
self.data = x.dataI confirmed that the original reproducer gives MemoryError on main, while this modification still allows it to be avoided. I don't know if there are "broadcasting" fast path considerations you'd like to make though vs. a hard shape match requirement, though the hard assignment of the data to self seems to imply a pretty strict requirement for shape similarity I think.
There was a problem hiding this comment.
Ah, very good point. This sorts the issue for now, and we can deal with broadcasting later.
perimosocordiae
force-pushed
the
perimosocordiae-patch-9
branch
from
f742e12
to
09a99ca
Compare
|
Ok, CI was passing and I believe the regression test/reproducer in the original issue was too slow to be practical when I tried it locally. |
|
I suppose we could follow up with an |
doronbehar
added a commit
to doronbehar/scipy
that referenced
this pull request
Apr 8, 2023
* main: (51 commits) ENH: stats.ttest_ind: add degrees of freedom and confidence interval (scipy#18210) DOC fix link in release notes v1.7 (scipy#18258) BLD: fix missing build dependency on cython signature .txt files DOC: Clarify executable tutorials and MyST/Jupytext ENH: stats: Add relativistic Breit-Wigner Distribution (scipy#17505) MAINT: setup.sh as executable file DOC: remove content related to `setup.py` usage from the docs (scipy#18245) DOC: orthogonal_procrustes fix date of reference paper and DOI (scipy#18251) BLD: implement version check for minimum Cython version ci: touch up wheel build action DOC: link to info about Gitpod. ENH: ndimage.gaussian_filter: add `axes` keyword-only argument (scipy#18016) BUG: sparse: Fix LIL full-matrix assignment (scipy#18211) STY: Remove unnecessary semicolon DOC: Pin docutils and filter additional sphinx warnings BUG: vq.kmeans() compares signed diff to a threshold. (scipy#8727) MAINT: stats: consistently return NumPy numbers (scipy#18217) TST: stats.dunnett: fix seed and filter warnings in test_shapes DOC: add info to use codespaces. MAINT: add devcontainer configuration ...
tylerjereddy
added a commit
to tylerjereddy/scipy
that referenced
this pull request
Apr 16, 2023
* also indentation fixes and add required shape check Co-authored-by: Tyler Reddy <tyler.je.reddy@gmail.com>
tylerjereddy
removed
the
backport-candidate
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reference issue
Fixes gh-18202.
What does this implement/fix?
Previously, LIL's
__setitem__would dispatch throughIndexMixin.__setitem__, which doesn't currently handle slice-based keys in an efficient way: it converts slices to index arrays. This means that the eventual call to LIL's overload of_set_arrayXarray_sparsewould not be able to detect the full-matrix assignment, and would thus densify the sparse matrix every time.Until we improve the IndexMixin side of things, this solves the issue by moving the fast-path detection higher up in the call stack, before the slices are converted.