TST: (numpy dev) fix return type for a selection of ufuncs applied to Column#15941
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
|
👋 Thank you for your draft pull request! Do you know that you can use |
52d0cc7 to
81acf68
Compare
|
Actually rebased onto #15929 so it's clearer what this PR brings. |
|
following the success of my experimental merger (#15949), let's rebase on main again so merging order can be left to reviewers. |
81acf68 to
8cb9f78
Compare
mhvk
left a comment
There was a problem hiding this comment.
Looks like the previous fix we had actually made other things worse... Here is a new suggestion...
astropy/table/column.py
Outdated
| # this is consistent with numpy 1.x behaviour for these functions | ||
| _function = context[0] if context is not None else None | ||
| if _function in (np.isfinite, np.isinf, np.isnan, np.sign, np.signbit): | ||
| if isinstance(out_arr, MaskedColumn): |
There was a problem hiding this comment.
this section can be
if isinstance(out_arr, BaseColumn):
out_arr = out_arr.data
Actually, never mind... see next comment
astropy/table/column.py
Outdated
| view_type = np.ndarray | ||
| out_arr = out_arr.view(view_type) | ||
|
|
||
| if (NUMPY_LT_2_0 or return_scalar) and ( |
There was a problem hiding this comment.
Actually, the problem is that the previous fix caused a breakage: np.isfinite is in _comparison_functions. So, we need to combine the two, as follows:
if self.shape != out_arr.shape or (
isinstance(out_arr, BaseColumn)
and (context is not None and context[0] in _comparison_functions)
):
data = out_arr.data
return data[()] if NUMPY_LT_2_0 or return_scalar else data
else:
return out_arr
There was a problem hiding this comment.
I'm not sure what breakage you've identified, and see no failing test with this branch (after rebase) locally.
For completion, I also don't see any failures with your suggestion applied.
I'll let CI complete here to see if I'm missing anything.
There was a problem hiding this comment.
Ok, all I see is ✅, and it's night time in France, so I'm logging off, but feel free to push your suggestion to my branch if needed ! Thank you !
What do you mean ? Is it because the number of failure is greater on this branch than you expect ? I'm going to rebase now, which should bring the number of failures to zero. While CI runs I'll also read your other comments. |
8cb9f78 to
d9253cc
Compare
d9253cc to
402720d
Compare
|
Well, not worse, but broke something that worked before. Anyway, the fix should now be easy -- I pushed a revised version up. |
mhvk
left a comment
There was a problem hiding this comment.
Should be good to go if tests pass!
|
Auto merge didn't work due to pytest 8 (sigh, the timing!). If the failures here are consistent with that I see in main with pytest 8, then I will merge. |
|
Thanks, all! |
…ection of ufuncs applied to Column
…941-on-v6.0.x Backport PR #15941 on branch v6.0.x (TST: (numpy dev) fix return type for a selection of ufuncs applied to Column)
Description
This pull request is to address part of #15926, specifically fixing
astropy/table/tests/test_column.py::TestColumn::test_numpy_boolean_ufuncs(as discussed in #15926 (comment))The fix is rather inelegant, but I'm not sure there's a better one.Close #15926