Fix bug where Quantity subclasses fail Table operations #11792
Conversation
taldcroft
changed the title
taldcroft
force-pushed
the
table-masked-quantity-operations
branch
from
177f5d3
to
e19a69b
Compare
| t1 = table.QTable([[1, 2, 3, 4], col], names=['a', 'b']) | ||
| t2 = table.QTable([[1, 2], col[:2]], names=['a', 'c']) | ||
|
|
||
| with pytest.raises(NotImplementedError) as err: | ||
| table.vstack([t1, t2], join_type='outer') | ||
| assert 'vstack requires masking' in str(err.value) | ||
| assert 'vstack unavailable' in str(err.value) |
There was a problem hiding this comment.
Change this to pytest.raises(NotImplementedError, match='vstack unavailable')?
There was a problem hiding this comment.
I noticed this old code, but was trying to keep the footprint small and not make changes just for style.
| @@ -584,7 +584,8 @@ def __setitem__(self, item, value): | |||
| if isinstance(value, Longitude): | |||
| raise TypeError("A Longitude angle cannot be assigned to a Latitude angle") | |||
| # first check bounds | |||
| self._validate_angles(value) | |||
| if value is not np.ma.masked: | |||
There was a problem hiding this comment.
I had hoped to avoid having non-masked classes to be aware of masks at all, but to avoid it here would mean creating a specific MaskedLatitutde class in utils.masked, which seems overkill. But the problem here is general, that classes may not be able to deal with np.ma.masked. I'll have a quick look to see if I cannot fix it more easily in Masked itself...
There was a problem hiding this comment.
That is an interesting question. In this case if there was going to be a MaskedLatitude class, wouldn't it belong in angles.py not in masked.py? I can imagine a MaskedLatitude class that just overrides _validate_angles to do nothing if value is np.ma.masked otherwise calls the super method.
In general classes might need some small tweaks like this to become masked, and these might be classes outside of astropy. What's the pattern for doing that?
There was a problem hiding this comment.
So, my current idea is that the top-level MaskedLatitude class (which gets created by Masked(Latitude)) should really deal with this, i.e., define a __setitem__. I tried that locally and it works, but I need to think a little bit exactly how best to implement this... But if I don't get to it, I'm quite fine with just doing it here for now.
There was a problem hiding this comment.
In the spirit of "perfect is the enemy of good", I'd say to keep this as is for now. There will be other natural opportunities that come up to improve the implementation.
|
Thanks for the quick reviews @mhvk. 🤞 this makes it into 4.3. |
|
And back to a previous comment, I am also really impressed at how easy it was to fully support Quantity, once the monster of #11127 was done. 🎉 |
…ions Fix bug where Quantity subclasses fail Table operations
|
backported to v4.3.x in 38b2a88 |
Description
This is a follow-on to #11127 to address a bug where Table operations fail on
Quantitycolumns where masking is required. Since #11127 masking is available with Quantity and so operations like hstack, vstack, and join should work. This bug was noted on Slack by @adrn.I fixed a couple of somewhat-related issues that I noticed while in the code.