Fix MaskedColumn TypeError from stale fill_value after dtype-changing ufuncs - #20265
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.
|
pllim
added
Bug
backport-v7.2.x
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
a2ac391 to
c7af2c4
Compare
No, upon further inspection that issue is more just a problem of how np.histogram deals with masked values in MaskedArrays, not an issue with the dtype between the array itself and the fill_value being different, as here. Yes, but I suspect there's a low chance of actually fixing that bug, given the sheer number of similar-ish bugs in |
CAOShurong
left a comment
CAOShurong
left a comment
There was a problem hiding this comment.
Independent exact-head verification review (AI assistance used and disclosed; tested locally on Windows, Python 3.13.1 / NumPy 2.5.2).
RED confirmed at base 3011554f (pristine main worktree): the reporter's exact snippet runs, but the failure is deferred, which matches why it looks "obscure":
col = MaskedColumn(["foo","bar","baz"], mask=False, fill_value="N/A", dtype="<U8")
r = np.strings.find(col, "foo") # returns int64 [0, -1, -1] — no error yet
r._fill_value # -> array('N/A', dtype='<U8') ← stale string fill on an int64 array
repr(r) # -> TypeError: Cannot convert fill_value N/A to dtype int64The TypeError also fires in realistic downstream use, not just repr: filtering a Table column by np.strings.find(t["uri"], ".jpg") == -1 (exactly the astroquery/MAST workflow from #20257) raises the same TypeError at base, and succeeds at this PR's head.
GREEN confirmed at head c7af2c41: np.strings.find and np.char.find both return [0, -1, -1] (int64) with _fill_value reset to None → materialized default 999999; repr/filled()/tolist() all fine. The masked-rows variant (mask=[False, True, False]) is cured too — at base that path leaves np.str_('N/A') as the fill of an int64 result (same latent bug, different trigger), while the patched tree resets it to the int default. So the fix covers more than the issue's minimal repro.
No-behavior-change checks (identical output on base vs head, i.e. the guard does not over-trigger):
- comparison ufunc
col > "bar"(U→bool dtype change): fill staysarray(True)on both trees; - non-dtype-changing ufuncs preserve custom fills exactly:
np.sinkeeps-1.0,np.abskeeps99; - scalar reductions (
sum,mean) unchanged; plain (non-masked)Columnunaffected.
Suites (head, editable-free detached worktrees with copied build artifacts): astropy/table/tests/test_column.py 147 passed / 2 xfailed (incl. the new regression test), full astropy/table 2425 passed / 201 skipped / 14 xfailed, astropy/utils/masked 1192 passed / 5 skipped / 2 xfailed. CI matrix fully green including towncrier.
Two minor, non-blocking notes:
ma.core._check_fill_valueis private NumPy API, and this appears to be its first use in the astropy codebase (git grepover main finds no prior use). It is stable across the supportednumpy>=2.0range today, but if astropy would rather not grow private-API surface, a small local validator (attemptout_arr.fill_value = fill_valueinside the same try/except) would behave equivalently — happy to test either way. Not a merge blocker from my side.- Test-gap suggestion: the new regression test covers the
mask=Falsecase only. Two cheap additions would lock in behavior I verified manually: (a) a masked-rows variant of the same ufunc (base currently leaves'N/A'as the fill there too), and (b) an assertion that a valid custom numeric fill survives a dtype-changing ufunc (e.g.MaskedColumn([1,2,3], fill_value=999999)throughnp.sqrtkeeps 999999) — that pins down that the guard resets only invalid fills.
On pllim's questions: yes — this is precisely a downstream mitigation for numpy/numpy#32401 (still open, zero comments as of today). Given the number of released NumPy 2.x versions users run, fixing it astropy-side is justified independently of whatever happens upstream in numpy.ma. And I agree with the author's read that #14558 is a separate root cause (masked-value handling inside np.histogram, not fill-value dtype propagation).
|
@jdavies-st - thanks! I added @mhvk for review since this is really his expertise. From what I saw here and in the numpy issue, it looks like we want to wait for the numpy fix to converge and then basically apply the same here. The other point would be to make the new code conditional on the numpy version so that it only runs for numpy < 2.6.0 (presuming the numpy fix gets into 2.6) and eventually we can remove the workaround. |
|
Thanks @taldcroft. Agree. Let's wait to see what happens with the numpy PR. I don't know what their backport policy is, so will have to see what is done for numpy < 2.6. |
|
Agreed with waiting and backporting your numpy fix once it is in. As the PR is labelled as a backport candidate, it probably will go to current numpy, so we'll need an |
|
@mhvk I think the numpy PR is complete and ready. Feel free to have a look over there (and review if you like), and if all looks good, I will update this PR to have |
c7af2c4 to
c931076
Compare
|
@mhvk I've updated this PR to align with the fix in Numpy. I used the NUMPY_LT_2_6 clause because even if it is backported to 2.5.x or 2.4.x, some minor versions of 2.5 and 2.4 will still have the bug. This should catch all of them. |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
@jdavies-st - only a comment on the changelog entry, otherwise it looks good.
Note the readthedocs failure -- I cannot really see why that would happen but maybe just in case rebase on current main when you make the change.
|
Hmm, saw the readthedocs failure in another recent PR now -- it's definitely unrelated, so don't worry about that! |
|
I see also that the devdeps job failed, and that is real. There seems to be something else going on in MaskedColumn.fill_value. Hmm. |
|
Oh, I see the devdeps numpy build being used actually lags from my commit fixing the bug on the numpy side, which means NUMPY_LT_2_6 correctly skips the fix here, but it also isn't in the numpy nightly build yet. So I think that's the issue. I would assume the nightly build will eventually catch up, but |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Thanks! Approving now, not sure why not all the tests are running.
|
Will close and reopen to trigger a new test run. |
62cbf5e to
6b54099
Compare
04a3c88 to
8997e88
Compare
|
So I rebased to main, and still see the failed test, same reason. The test running is using an out-of-date version of the Perhaps this is a bug with the astropy CI infrastructure? Is there some pip or uv package cache that is preserved across runs? It is caused at least partially by the fact that numpy doesn't rename their artifacts (or the versions for that matter). It is always Anyway, I will investigate further. |
|
Let me ping @neutrinoceros about the UV cache, as we discussed this just a little while ago (in the reverse direction, that it was a pity the cache was not used...); @neutrinoceros - see message just above. |
|
Ah, ok! Well I just went down this rabbit hole, so let me report. the setup-uv cache is actually getting left to the default And then later And since for this repo And since the version for numpy (and possibly other devdeps) is .dev0 always, then it just uses the cached version, and it's impossible to tell in the log. I would argue that caching should be turned off for all devdeps jobs, because it's not testing against what you think it is. |
|
I'll do a quick check in this PR by setting UV_NO_CACHE on the devdeps jobs in EDIT: I realize this is the same as #20263. |
|
Ok, that worked! To be clear, this one-line change to Also I turned off I also noticed that UV_INDEX_STRATEGY = unsafe-best-matchis set for |
|
Alas the UV stuff might get in the way of auto backports, but let's see... 🤞 Thanks! |
This comment was marked as resolved.
This comment was marked as resolved.
|
Happy to pull the UV |
This comment was marked as resolved.
This comment was marked as resolved.
|
I already have PRs in the air to address caching friction, no need to open new ones ! |
|
For the record: #20285 |
|
I also opened #20336 as a follow-up. FYI. Sorry, @neutrinoceros . Hopefully the one liner here is easy for you to follow up on in your relevant PRs. |
|
yeah. It's now cleaned up in #19886. I'm considering self-merging it as it's becoming more work for everyon to keep it open than not |
…after dtype-changing ufuncs (#20335) Co-authored-by: James Davies <[email protected]>
|
For completeness: The rest of backport is at #20338 |
…skedcolumn-fillvalue Fix MaskedColumn TypeError from stale fill_value after dtype-changing ufuncs (cherry picked from commit dd8cd5e)
6 participants
Description
This fixes a
np.ma.MaskedArrayquirk where__array_wrap__copies the input's rawfill_valueonto the output without checking it still matches the possibly changed output dtype.MaskedColumn.__array_wrap__now re-validates_fill_valueagainst the ufunc output's dtype, resetting it to the default when invalid.This bug exists since numpy 2.0, which returns a masked array for these ufuncs. Before that, a standard ndarray was returned.
Fixes #20257