Fix bug in sparse matrix indexing #1651
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
|
Checking updated PR...
Comment last updated at 2024-08-12 09:24:28 UTC |
Previously, they could also be stored as plain lists, which would happen whenever the COOMatrix objects was initialised with lists. Most operations still worked ok then, but equality check means something completely different for NumPy arrays, and ProductSpaceOperator's __getitem__ method relies on this behaviour to look up the desired linear index for a requested element in the matrix of operators. If the index is a list, this would give wrong results.
leftaroundabout
force-pushed
the
bug/sparsematrix-indexing
branch
from
319af32 to
3b4a7db
Compare
Emvlt
reviewed
Aug 27, 2024
There was a problem hiding this comment.
This class is a custom implementation of sparse COO arrays, for the special case of sparse arrays of operators, feature which scipy does not support anymore. This feature is important in methods such as Kaczmarz algorithms, hence this class. The type checking was not sufficient, for using list to declare the non zeros elements of the array would then cause issues downstream. So Justus had to convert the list to an array from the beginning.
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.
As per #1650, the new implementation of
ProductSpaceOperatorafter #1642 has a problem with element-wise accessing of the constituent operators, specifically with the indexing into the matrix of these operators.I traced the problem to Python's lack of type safety - specifically the fact that the new
COOMatrixclass does not guarantee its row- and column indices are stored as NumPy arrays asProductSpaceOperatorrequires.A simple explicit conversion through
np.asarrayfixes this problem.