Define a mixin for container classes with possibly masked values and use it in Time by mhvk · Pull Request #16844 · astropy/astropy

mhvk · 2024-08-17T23:38:31Z

I've started implementing proper masks for representations, roughly following what is done for Time. To help that, this PR defines a new MaskableShapedLikeNDArray mixin that provides some basics (like unmasked and filled), and also some helper routines (built on top of ShapedLikeNDArray, which all container classes already use).

In this PR, this is only used to somewhat simplify the implementation in Time, but I will use it in follow-up for coordinates. I on purpose did not yet add a changelog entry for the new method, since I'd like to keep it semi-private until I'm sure it is the right approach also for coordinates -- the fact that it is now used in Time will help ensure I keep the masking implementations consistent.

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2024-08-17T23:38:58Z

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.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

mhvk · 2024-08-17T23:39:19Z

-        mask = False
-        mask, val_data = get_mask_and_data(mask, val)
-        mask, val_data2 = get_mask_and_data(mask, val2)
+        data, masks = self._get_data_and_masks(val, val2)


These methods come with MaskableShapedLikeNDArray

mhvk · 2024-08-17T23:39:41Z

    def masked(self):
        return isinstance(self._time.jd1, Masked)

-    @property


These are now provided by MaskableShapedLikeNDArray.

mhvk · 2024-08-17T23:40:13Z

    return val


-def get_mask_and_data(mask, val):


This basically repeated code in masked, so is now superfluous.

mhvk · 2024-08-17T23:40:37Z

+    but also used in `~astropy.utils.masked.Masked`.
+    """
+
+    @classmethod


All four methods are simply copied from Masked and MaskedNDArray.

mhvk · 2024-08-22T19:32:16Z

@taldcroft - I realize you may be using your astropy time mostly for tables, but if you have a chance, I'd appreciate a look at this PR factoring out some of the Time mask-related methods -- with the idea to use them for coordinates (for representations, see #16845). Thanks!

taldcroft · 2024-08-22T23:15:54Z

@mhvk - I will have a look, but on first glance this will require my thinking hat so not this evening!

taldcroft

Overall this looks good and I see now that it is basically an uncontroversial refactoring.

One thing for discussion is making the private utility class methods here into public functions. Unless I missed something, none of them actually use cls anywhere, and they are somewhat generic for masks and a masked protocol or np.ma.MaskedArray.

taldcroft · 2024-08-24T12:45:07Z

+        mask = getattr(data, "mask", None)
+        if mask is not None:
+            try:
+                data = data.unmasked
+            except AttributeError:
+                if not isinstance(data, np.ma.MaskedArray):
+                    raise
+                if data is np.ma.masked:
+                    if allow_ma_masked:
+                        data = None
+                    else:
+                        raise ValueError("cannot handle np.ma.masked here.") from None
+                else:
+                    data = data.data


It's a bit out of scope (since this PR is mostly a refactor), but I have some suggestions on the code.

Remove a big code indent with an early return.

Avoid unnecessary re-use of data. With a mindset of type-annotated code, redefining a variable seems like an idiom to avoid when possible. In this case I think the code is much more clear.

Improve exception messages

Suggested change

mask = getattr(data, "mask", None)

if mask is not None:

try:

data = data.unmasked

except AttributeError:

if not isinstance(data, np.ma.MaskedArray):

raise

if data is np.ma.masked:

if allow_ma_masked:

data = None

else:

raise ValueError("cannot handle np.ma.masked here.") from None

else:

data = data.data

mask = getattr(data, "mask", None)

if mask is None:

return data, mask

try:

unmasked = data.unmasked

except AttributeError:

if not isinstance(data, np.ma.MaskedArray):

raise TypeError(

"data must have an 'unmasked' attribute or be np.ma.MaskedArray"

) from None

if data is np.ma.masked:

if allow_ma_masked:

unmasked = None

else:

raise ValueError(

"cannot handle np.ma.masked here, use allow_ma_masked=True"

) from None

else:

unmasked = data.data

return unmasked, mask

I went with your suggestion to make this function public, and decided that, given that, I should also simplify it. So, I followed your suggestion to change the indentation, but removed the option of dealing with np.ma.masked (which should have included np.ma.nomask anyway).

taldcroft · 2024-08-24T12:46:43Z

+        ----------
+        data : array-like
+            Possibly masked item, judged by whether it has a ``mask`` attribute.
+            If so, checks for being an instance of `~astropy.utils.masked.Masked`


Suggested change

If so, checks for being an instance of `~astropy.utils.masked.Masked`

If so, checks for having an ``unmasked`` attribute

Adjusted text, thanks!

taldcroft · 2024-08-24T12:52:36Z

+        return data, mask
+
+    @classmethod
+    def _get_data_and_masks(cls, *args):


I would suggest renaming this to _get_datas_and_masks. I really like the idiom of consistently using plural for list-like objects. This gives a strong type hint to the reader and makes iteration over the object obvious to code using the singular.

Maybe also use arrays instead of args if this will be public. Overall in some of these functions I feel like array is a better name than data since that name narrows the type considerably. But that may be sliding into "perfect is the enemy of good" territory.

Sorry, should have answered here: I ended up thinking that the plural was not needed as much (or at least it wasn't clear it was going to be needed a lot), so just left this on the class to be used by __array_ufunc__.

p.s. Did go with array in the general get_data_and_mask

mhvk · 2024-08-24T23:52:03Z

Thanks, @taldcroft! I had been wondering about functions versus class methods, but your comment made clear functions is the answer. The only annoyance is that I couldn't stop myself from then using the function also in the function_helpers... (indeed, this uncovered a bug, though I'll deal with that in a separate PR...)

taldcroft

@mhvk - thanks, this is looking nice now! Just one substantive question and then documentation or nits.

taldcroft · 2024-08-25T09:55:03Z

+    copy : bool optional
+        Whether to ensure a copy is made. Only relevant if a single
+        input mask is not `None`, and ``out`` is not given.
+    """


Needs a Returns.

taldcroft · 2024-08-25T09:58:04Z

+        Parameters
+        ----------
+        fill_value : object
+            Value to replace masked values with.  Note that if this value is masked


Finish the sentence and add a Returns.

How weird - that was already unfinished in Time... I guess it should have been that the mask is ignored, but to be honest, I feel that is such a weird corner case, it is better not even to mention it... But added a Returns section.

taldcroft · 2024-08-25T10:12:54Z

-            np.logical_or(out, mask, out=out, where=where)
-        return out
+    @staticmethod
+    def _get_data_and_masks(arrays):


Since it is so easy with Copilot it's worth a docstring here.

""" Extracts the data and masks from the given arrays. Parameters ---------- arrays : iterable An iterable of arrays. Returns ------- datas, masks A tuple of extracted data arrays and mask arrays, respectively. """

OK, thanks. Used slightly adjusted.

taldcroft · 2024-08-25T10:19:41Z



-def _get_data_and_masks(*args):
+def _get_data_and_mask(array):


Optional, but maybe naming this _get_data_and_mask_as_array might help make the intent clear in the code that calls this.

Yes, good suggestion -- as is, it is just confusing that the name is the same with just an underscore added. I made it _get_data_and_mask_array (saving the plural for the other routine).

mhvk · 2024-09-11T17:50:56Z

@taldcroft - would you have time to do a final review for this refactoring of Masked and its use in Time? I hope to now use it all in coordinates...

taldcroft

This looks good now from the code perspective.

Since it adds 3 items to the masked subpackage public API, there should be a changelog entry as such. It might also be worth mentioning that the Time base class now inherits from MaskableShapedLikeNDArray, mostly to advertise how this new formalism can be used elsewhere.

You may even want a What's New entry for this to advertise this nice addition. If you get the coordinates version through for 7.0 then definitely this will need highlighting.

…sses. As part of this, moves _combine_masks and some other class methods from Masked to a new MaskedMethods class so that they can be reused by MaskableShapedLikeNDArray as well.

mhvk · 2024-09-14T20:19:45Z

OK, I added a changelog entry following your suggestion. I really hope to have the changes to coordinates done before 7.0 - and that would definitely warrant a what's new in which I will mention the more general scheme one can now use.

taldcroft

Looks great, thanks!

mhvk added time Refactoring no-changelog-entry-needed utils.masked labels Aug 17, 2024

mhvk added this to the v7.0.0 milestone Aug 17, 2024

mhvk requested a review from taldcroft as a code owner August 17, 2024 23:38

github-actions Bot added the Docs label Aug 17, 2024

mhvk commented Aug 17, 2024

View reviewed changes

mhvk mentioned this pull request Aug 18, 2024

Allow representations to be masked, with possibly different masks for the components #16845

Closed

1 task done

mhvk removed the Docs label Aug 22, 2024

taldcroft reviewed Aug 24, 2024

View reviewed changes

mhvk force-pushed the maskable-class-use-in-time branch from 0a7f5d6 to 816d41f Compare August 24, 2024 23:46

mhvk force-pushed the maskable-class-use-in-time branch from 816d41f to f74be70 Compare August 25, 2024 00:12

taldcroft reviewed Aug 25, 2024

View reviewed changes

mhvk force-pushed the maskable-class-use-in-time branch from f74be70 to 9746745 Compare August 25, 2024 13:03

mhvk force-pushed the maskable-class-use-in-time branch 2 times, most recently from c9fc542 to 4922c27 Compare September 11, 2024 17:33

taldcroft requested changes Sep 14, 2024

View reviewed changes

taldcroft removed the no-changelog-entry-needed label Sep 14, 2024

mhvk added 4 commits September 14, 2024 16:18

Create a new MaskableShapedLikeNDarray class for use in container cla…

849269c

…sses. As part of this, moves _combine_masks and some other class methods from Masked to a new MaskedMethods class so that they can be reused by MaskableShapedLikeNDArray as well.

Use the MaskableShapedLikeNDArray class in time.

b6e980f

Use Masked private methods to simplify merging of val, val2 masks

1e3a5f7

DOC: add changelog entry explaining API changes.

f7f212e

mhvk force-pushed the maskable-class-use-in-time branch from 4922c27 to f7f212e Compare September 14, 2024 20:18

taldcroft approved these changes Sep 15, 2024

View reviewed changes

mhvk merged commit e5fa5cb into astropy:main Sep 15, 2024

mhvk deleted the maskable-class-use-in-time branch September 15, 2024 11:28

mhvk mentioned this pull request Sep 15, 2024

BUG: fix how np.put, putmask, place, copyto deal with masked, nomask #17014

Merged

1 task done

	If so, checks for being an instance of `~astropy.utils.masked.Masked`
	If so, checks for having an ``unmasked`` attribute



		def _get_data_and_masks(*args):
		def _get_data_and_mask(array):

Conversation

mhvk commented Aug 17, 2024

Uh oh!

github-actions Bot commented Aug 17, 2024 • edited by taldcroft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Aug 22, 2024

Uh oh!

taldcroft commented Aug 22, 2024

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk Aug 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Aug 24, 2024

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Sep 11, 2024

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk commented Sep 14, 2024

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Aug 17, 2024 •

edited by taldcroft

Loading

mhvk Aug 24, 2024 •

edited

Loading