BUG: sparse: raise error for array classes, document/test old behavior #20490
Conversation
ilan-gold
changed the title
lucascolley
changed the title
lucascolley
added
the
defect
There was a problem hiding this comment.
The diff looks pretty clean to me, and it seems that CJ gave a +1 to the idea of errors in these cases in the matching issue.
There are a few related linter complaints you can see in the CI.
I should let CJ or another sparse regular take a look before it goes in though, but looks pretty clean at first glance.
scipy/sparse/_base.py
Outdated
| def __init__(self, *args, **kwargs): | ||
| if np.isscalar(args[0]): | ||
| raise ValueError( | ||
| "scipy sparse array classes do not \ | ||
| support instantiation from a scalar" | ||
| ) | ||
| super().__init__(*args, **kwargs) |
There was a problem hiding this comment.
The __init__ methods are pretty spread out across classes already. If it spreads more it may become hard to tell which init methods gets called during the init process. So, I think in the long run this check might be better placed inside _sputils.check_shape. But that's just my suggestion. I'm ok with other choices.
@perimosocordiae should probably decide.
Co-authored-by: Dan Schult <dschult@colgate.edu>
…gold/scipy into ig/sparse_array_singletons
scipy/sparse/_bsr.py
Outdated
| f'mismatching blocksize={blocksize}' | ||
| f'vs {self.data.shape[1:]}' |
There was a problem hiding this comment.
You probably want a space either at the end of the first string or at the start of the second string. I typically put it at the start of the second string but that's personal preference. If the space isn't in either spot the {blocksize}' output runs into the vs`.
| self._shape = None | ||
| if self.__class__.__name__ == '_spbase': | ||
| raise ValueError("This class is not intended" | ||
| " to be instantiated directly.") | ||
| if isinstance(self, sparray) and np.isscalar(arg1): |
There was a problem hiding this comment.
Implementing the suggestion from our call means that _spbase now needs to check the first argument which resulted in this change. Not sure if the lack of arguments in _spbase was a choice or not, but maybe there's a way around this?
There was a problem hiding this comment.
Hmmm... Good point about lack of arguments being a choice or not.
It looks like setting maxprint actually doesn't work for all the subclasses that override __init__. So currently we can't choose a non-default maxprint value for most of the formats.
That could be fixed by adding the maxprint parameter to __init__ methods throughout. Or by adding **kwargs parameters that get passed through to the superclass __init__. The second option is harder to read for someone trying to figure out what all the options are.
And this could be fixed in a separate PR. Let me know if you prefer that and I'll open a separate issue.
There was a problem hiding this comment.
Yeah, the maxprint functionality has been inaccessible for a while, unfortunately. We should clean that up in a later PR.
| self._shape = None | ||
| if self.__class__.__name__ == '_spbase': | ||
| raise ValueError("This class is not intended" | ||
| " to be instantiated directly.") | ||
| if isinstance(self, sparray) and np.isscalar(arg1): |
There was a problem hiding this comment.
Yeah, the maxprint functionality has been inaccessible for a while, unfortunately. We should clean that up in a later PR.
|
This PR looks good from here. Once the CI completes, let's merge it. |
|
Merged, thanks @ilan-gold for the PR! |
Reference issue
Closes #20128
What does this implement/fix?
We raise an error on all array classes and ensure that matrices continue to return 1x1 objects.
Additional information
The way I did this was to make
sparraythe first parent in the super classes and add an__init__method. Not super familiar with the codebase, but presumably since nothing broke, this is an ok thing to do?