| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Time.__setitem__ wrote the value's jd1/jd2 straight into the target's plain ndarrays, so if the target was not yet masked the incoming mask was dropped and the masked rows showed their underlying values again. Upgrade jd1/jd2 to Masked first when the value is masked, mirroring what setting np.ma.masked already does. This is what made table vstack lose the mask on Time mixin columns: it builds the output with TimeInfo.new_like (unmasked) and then fills it via setitem. Fixes astropy#20173
|
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.
|
Sorry, something went wrong.
|
@Mohit-Ak - first, thanks for the contribution. I pushed one commit to refactor some common code into a private method on TimeFormat. One issue is that now Masked(jd1, copy=False) shares the data buffer but allocates a fresh mask array, so a view's mask never reaches the parent. import numpy as np from astropy.time import Time t = Time(["2001:001", "2001:002", "2001:003", "2001:004"]) s = t[1:3] # view sharing t's jd1/jd2 buffers print(np.shares_memory(t._time.jd1, s._time.jd1)) # True masked_value = Time(["2010:001", "2010:002"]) masked_value[1] = np.ma.masked s[:] = masked_value print(s.mask) # [False True] print(t.masked) # False print(t.value[2]) # 2010:002:00:00:00.000 <-- should be hidden This seems unavoidable when promoting to Masked, as the parent is unmasked and the view is masked. @mhvk - do you have any thoughts on the right behavior? |
Sorry, something went wrong.
|
Another issue is that a failed assignment can convert to masked but then not actually do the assignment. import io, numpy as np
from astropy.table import QTable
from astropy.time import Time
t = Time(["2001:001", "2001:002", "2001:003", "2001:004"])
v = Time(["2010:001", "2010:002"]); v[1] = np.ma.masked
print(t.masked) # False
try:
t[:3] = v # shape mismatch
except ValueError as exc:
print(exc) # could not broadcast input array from shape (2,) into shape (3,)
print(t.masked, t.mask) # True [False False False False]
|
Sorry, something went wrong.
|
@taldcroft - this is quite tricky, and I don't have a ready answer. FWIW, np.ma.MaskedArray(array) works the same way: if you set an element to something masked, the value in array will change too, but it will of course not get masked. In principle, in Time, we do have information on whether we are a slice or own our own data, so in that sense we can protect users by, e.g., not allowing a view to become masked (or warning or whatever); see the end of _apply and the use of _id_cache to ensure that caches get cleared if something is written too. p.s. That Time becomes masked and an assignment then fails is clearly a bug. Thankfully, one that is not too difficult to fix. |
Sorry, something went wrong.
There was a problem hiding this comment.
Now also looked at the actual PR: I think this looks good! Two comments, though the long first one here ends up boiling down just to a request to change the name, to _ensure_masked.
Sorry, something went wrong.
| self._time.jd2 = Masked( | ||
| self._time.jd2, mask=self._time.jd1.mask, copy=False | ||
| ) | ||
| self._time._convert_to_masked() |
There was a problem hiding this comment.
The name of the method suggests that something will always happen, but it should only do something when we're not masked, so maybe self._time._ensure_masked()?
Note that I really like that this is put to the TimeFormat class; better separation of concerns.
Actually, another suggestion: how about creating a TimeFormat.masked property which can be set? So, here, it would be self._time.masked = True (but it would not be possible to set it to False). That could then also be used inside the masked property here, separating concerns. Though I think this would be better done as follow-up; it doesn't really matter...
Sorry, something went wrong.
There was a problem hiding this comment.
Agreed on the name.
Sorry, something went wrong.
There was a problem hiding this comment.
And yes, let's leave the property for later.
Sorry, something went wrong.
| # If the value carries a mask but we do not, we have to upgrade our | ||
| # internal jd1/jd2 to Masked first, otherwise the mask of the value | ||
| # would be silently dropped (gh-20173). | ||
| if isinstance(value._time.jd2, Masked): |
There was a problem hiding this comment.
Here, we know value is a Time instance, so just if value.masked:
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Description
Fixes #20173.
vstack was silently dropping the mask on Time mixin columns, but the root cause turned out to sit one level down, in Time.__setitem__.
The tail of __setitem__ writes the value's jd1/jd2 straight into the target's arrays:
If the target Time is not masked yet, those are plain ndarrays. Assigning a MaskedNDArray into a plain ndarray keeps the data and throws the mask away, so the masked entries come back as their underlying (pre-mask) values with no error or warning.
That is exactly the path vstack takes. _vstack builds the output column with TimeInfo.new_like, which allocates jd1 = np.full(shape, jd2000) / jd2 = np.zeros(shape) — deliberately unmasked, since it is meant to be filled in place — and then fills it with col[idx0:idx1] = array[name]. The first assignment silently loses the mask.
Worth noting join and hstack are unaffected: they index the source column directly (array[name][array_out]) instead of round-tripping through new_like + setitem, so their masks survive. That is why this only showed up in vstack.
The fix upgrades jd1/jd2 to Masked before the assignment when the incoming value is masked and we are not. This mirrors what the value is np.ma.masked branch a few lines above already does, and reuses the same mask=self._time.jd1.mask sharing so jd1 and jd2 keep a common mask.
I put the fix in __setitem__ rather than in new_like or _vstack on purpose. Making new_like always return a masked Time would force a mask onto every join/vstack output whether or not anything is masked, and patching _vstack alone would leave the underlying setitem hole open — plain t[:2] = masked_time loses the mask too, independent of tables:
Unmasking still behaves as before — assigning an unmasked value over a masked element clears that element's mask, and the existing np.ma.nomask branch is untouched.
How was this tested?
Two regression tests in astropy/time/tests/test_mask.py: test_setitem_masked_value covers the setitem behaviour directly (slice assignment, scalar assignment of a masked element, shared jd1/jd2 mask, and that unmasking still works), and test_vstack_masked is the reporter's scenario from the issue.
Both fail on main and pass with the fix:
$ python -m pytest astropy/time/tests/test_mask.py::test_setitem_masked_value \ astropy/time/tests/test_mask.py::test_vstack_masked -q # before: 2 failed (assert t.masked -> AssertionError: assert False) # after: 2 passedThe reproducer in the issue now prints the expected output:
Full suites, no regressions:
$ python -m pytest astropy/time/ 1002 passed, 30 skipped, 9 xfailed $ python -m pytest astropy/table/ 2407 passed, 189 skipped, 14 xfailed $ python -m pytest astropy/utils/masked/ astropy/timeseries/ astropy/io/misc/ 3845 passed, 269 skipped, 2 xfailedLint with the pinned ruff (v0.15.20 from .pre-commit-config.yaml):
I'll add the changelog fragment once this has a PR number.