BUG: Preserve mask in Time.insert for masked values - #20232
Open
taldcroft wants to merge 5 commits into
Open
Conversation
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
Contributor
|
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.
|
mhvk
reviewed
Aug 10, 2026
mhvk
left a comment
mhvk
left a comment
Contributor
There was a problem hiding this comment.
Nice! A suggestion to make it simpler still.
| # case of a string input that has a length which is not the length we want). | ||
| if not isinstance(values, self.__class__): | ||
| values = np.asarray(values) | ||
| # asanyarray, not asarray, so that a Masked input keeps its mask. |
Contributor
There was a problem hiding this comment.
How often have I asked this over at numpy, only to find it in our own code! Nice that it is so simple, though!
| [self], len(self) + n_values, name=self.info.name | ||
| ) | ||
|
|
||
| # ``new_like`` always makes an unmasked object, so if self is masked then |
Contributor
There was a problem hiding this comment.
An alternative is to rely on Time.__setitem__, which will now do this for you, i.e.,
out[:idx0] = self[:idx0]
(and same further down). It would be slightly slower (but only slightly), but definitely clearer.
Member
|
Hmm this is milestoned for backport but #20178 is not. So which milestone is correct? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Time.insertsilently dropped the mask, exposing values that should have stayed hidden. This is also reachable fromTable.add_rowon aTimecolumn (see #20230).This PR upgrades the output to
Maskedwhenselfis masked, using theTimeFormat._convert_to_maskedhelper added in #20178, and usenp.asanyarrayso aMaskedinput keeps its mask.Fixes #20230
Important
This is stacked on #20178 — so that needs to be merged first. The diff here is a single commit;
the extra commits shown are #20178's.
AI disclosure
This PR was initially created using Claude Opus 5, but I then edited the tests a fair bit to my liking. I understand all the changes and they match what I would have done (just more slowly).