Masked frames and SkyCoord - #17016

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.

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

I'm taking this out of draft given the upcoming feature-freeze deadline -- if we miss that, masked SkyCoord will not be possible until astropy 8.0.

I think this is complete if somewhat minimal, with the main question whether one should go beyond simply propagating masks (as done here). E.g., what should expected behaviour be for coordinate conversions and methods like apply_space_motion for spherical representations with some masked distances, PMs, and radial velocities (see also on top).

My own 2¢ is that we should simply go with the minimal implementation I have, mark coordinate masks as experimental, and allow masks not to propagate in some specific cases later.

cc @taldcroft, @StuartLittlefair, @adrn, @eerovaher, @nstarman

I like the look of this a lot, but I'm not sure about the special treatment of spherical coordinates to disregard the distance. A few things: 1. If a user passes a masked distance and unmasked angles I think they would be very surprised to not find that in the total mask. 2. I think if a user wants to mark a distance as truly missing they should use Nan. Which of course can be combined with a mask, but these have different meanings, right?

I like the look of this a lot, but I'm not sure about the special treatment of spherical coordinates to disregard the distance.

Thanks for bringing that up! It is the thing I have most struggled with, what exactly to do with missing distances, as well as incomplete differentials (proper motions and radial velocities). The basic problem is that a SkyCoord often just has RA, Dec, and for many catalogues there will be more informationfor only a subset of the data. But RA, Dec is enough to know where to point one's telescope!

For the mask, I hope my .get_mask() function provides the flexibility to get what exactly a user wants, but it is then a question of defaults, with two most logical options:

1. .mask tells whether "one can point a telescope" and default .get_mask() combines the masks of everything.
2. .mask combines everything, while .get_mask("ra", "dec") (perhaps the default) just does RA, Dec.

Which one would you prefer?

p.s. Part of the reason to have masks is to allow table joins in which one table does not have the given SkyCoord column. For that, of course, just any mask will do.

Which one would you prefer?

I lean towards the 2nd one. In the lower classes .mask combines all masks. IMO we should keep that pattern and make sure that other common use cases are deeply supported.

Have you thought about how SkyCoord.apply_space_motion() should work with masked time? I don't see any tests for that.

Have you thought about how SkyCoord.apply_space_motion() should work with masked time? I don't see any tests for that.

If I'm right, it will "just work", i.e., propagate, but let's add a test!

Test added - it works as expected.

I lean towards the 2nd one. In the lower classes .mask combines all masks. IMO we should keep that pattern and make sure that other common use cases are deeply supported.

Just to be sure: still exclude in the mask possible attributes, right, i.e., effectively just return self.data.mask (would be consistent with Time).

One big advantage of this approach is that it gives a bit more leeway in postponing worrying about how, e.g., apply_space_motion could special-case masked distances and radial velocities. It might then make more sense to simply add an argument to the method about how to deal with masked data (not sure what is best, but we would now have the benefit of not having to decide too much...).

Anyway, I can go either way. Would be good to have input from others, as the question of how to deal with different masks for different coordinate parts is really at the heart of it all.

Just to be sure: still exclude in the mask possible attributes, right, i.e., effectively just return self.data.mask (would be consistent with Time).

That's an interesting Q. Have to think on that more. I would say that if we're marking this as an experimental feature then yes, for now. We can revisit the question of attributes after v7.

It might then make more sense to simply add an argument to the method about how to deal with masked data (not sure what is best, but we would now have the benefit of not having to decide too much...).

Agreed, if I'm interpreting this right and you mean figuring out the added arguments can happen post v7.
The alternative to added args is a context manager approach, which might also be good

e.g.

with mask_propagator(mode="on-sky"):
    ...

But can we decide this for v7.1?

OK, now implemented a default mask that combines representation and possible differentials. It makes things quite a bit simpler, so that argues that it was the right change to make.

Check PR changelog error is because this is based on the mask for representations PR, #16845, which is not yet merged.

Based on the tests and the documentation the public API seems mostly reasonable. The one question I have is whether the output of get_mask() should be a structured array so that its components could be accessed by the name of the corresponding attribute. This would be especially useful when get_mask() is called without any arguments.

The way this is written, I think not - it is meant to give a array of plain bool back. It would not be too hard to extend (e.g., with a structured=True/False keyword argument), but I'm not sure I see a big benefit over just accessing the relevant masks directly.

In particular there is very little support for structured arrays outside of numpy. Supporting it as an option is fine, less so as the default.

Let me take this out of the thread, since it is less about .filled() than about __setitem__: basically, as implemented, if one has an unmasked representation (or frame or SkyCoord) and uses sc[...] = other_sc, then if other_sc is masked, sc will become masked too. This in itself seems fairly logical, but it is not how it works for ndarray (where we have no choice: we cannot override __setitem__, and numpy just ignores the mask), nor for Quantity (which by itself does not know about masks, and cannot change its own class to MaskedQuantity anyway).

For Time, we do check whether the value is masked, so it does not become masked either. So, options would seem to be:

1. Adjust the PR here to ignore the mask for __setitem__ too;
2. Adjust Time to take the mask into account;
3. Live with the inconsistency.

I really do not like (3). Maybe the cleanest (as far as possible) is to go with (1) for now - I am traveling next 2 weeks but can probably do a quick bugfix for both Time and coordinates if we want to go with (2) instead.

OK, did the push, and tests pass. Very happy to see that table joining now works properly!

And, just to reiterate it, I'm very much in favour of following this up with adding other methods like fill_attrs or a set_mask or so.

Thanks, @nstarman, @eerovaher, @taldcroft and @StuartLittlefair - it's going in!