Repository navigation
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.
|
f0a0875 to
5719fd9
Compare
f340ac3 to
de1f7b5
Compare
de1f7b5 to
236a7f5
Compare
|
@taldcroft, @nstarman (for masking in general) and @StuartLittlefair, @adrn, @eerovaher (coordinate maintainers): this PR introducing masked representations is ready for review. Most important perhaps is the big picture question of whether you think my logic about not sharing masks is correct (and whether, for follow-up on frames and |
|
Follow-up PR using this in coordinate frames and |
nstarman
left a comment
nstarman
left a comment
There was a problem hiding this comment.
Great to see more masked support!
|
|
||
| @property | ||
| def masked(self): | ||
| return isinstance(getattr(self, self.components[0]), Masked) |
There was a problem hiding this comment.
I know that it's designed s.t. f one field is masked all are masked, but with early exiting the following should be equivalent speed-wise 🤞🏼
| return isinstance(getattr(self, self.components[0]), Masked) | |
| return any(isinstance(getattr(self, c), Masked) for c in self.components) |
There was a problem hiding this comment.
Not sure: if this gets out of sync, there are arguably larger problems than whether .masked is still OK - e.g., any assignments with masked values will silently drop the mask (numpy problem, not something easy to catch...).
But happy to change it too...
There was a problem hiding this comment.
It's not a large priority, we're just not built around immutable data structures.
I think so! |
I actually have a separate branch in which I tried to keep the angular masks shared. In the end, I felt it was too fragile, e.g., in having to check for shared masks also in setting, resetting masks after a conversion from cartesian to spherical, etc. Also, I worried I might be too presumptuous about what a representation would look like (though I tried to give flexibility). E.g., should the masks on r and z in cylindrical coordinates also be merged? In the end, it is probably also clearer for the user if each element simply has its own mask. Coordinates are a bit different from |
|
I still think that the ability to treat the masks of the on-sky coordinates and the masks of proper motion components together would be user-friendly, but a better way to implement that would be to have convenience methods on the specific representation and differential classes and that should be done in a separate pull request to keep the scope of this one manageable. If your question was whether there should be a single mask for the entire representation or separate masks for the separate components then the nature of astronomical observations dictates that separate masks would be much more useful. |
|
OK, good point that one can separate the concerns: this PR indeed does not preclude later having some sharing of masks between the angular coordinates. Indeed, it may be helpful to rebase my branch that attempted to implement shared masks on this once it is merged; that will make clearer what the differences are. |
|
Opening a few small self-contained pull requests instead of a single very large one is usually a good idea. However, in this case it seems to me that the best way to ensure we are coming up with a good API is to ensure Large pull requests can be made more manageable by keeping the commit history clear and atomic, and from a quick glance at #17016 it seems like you've been doing that already, so I don't see any value in reviewing this pull request separately. |
|
@eerovaher - if it is easier for you to review the whole (i.e., #17016, which is built on this), that is definitely fine by me! I can add extra tests, etc., wherever is most appropriate. |
|
|
|
Good point. I'll add an |
53fabbd to
f720d53
Compare
| self._ensure_masked() | ||
|
|
||
| if set_mask or clear_mask: | ||
| for comp in self.components: |
There was a problem hiding this comment.
Unrelated to this PR but maybe we should have a _components attribute that provides an iterator for the _-prefixed components.
There was a problem hiding this comment.
Except the differentials repetition (https://github.com/astropy/astropy/pull/16845/files#r1797307453) which will be fixed, this LGTM.
Obviously the coordinates maintainers should have final say and @eerovaher is making sure space motion is working as expected.
Thanks @mhvk!
I pushed the fixed version that I already had in #17016 here too, so that this can undergo final review. |
e37b528 to
24db0cb
Compare
|
Before I start reviewing this in detail I need to ask why should I review this at all if #17016 exists? Shouldn't I review just that? |
|
This PR introducing masks for representations is now ready for final review -- I really hope to get this in before feature freeze! I think it is now independent of the larger outstanding question of how exactly to deal with masks in After review of this one, please do look at #17016 (comment) and help decide what is the best path forward (ideally also in time for feature freeze - the choices so far are not difficult in terms of code changes in the PR). |
You can certainly review #17016 if that works better for you - and I'm happy to "back-port" any suggestions you make there for |
|
Following #17016 (comment), pushed an extra commit with a test showing that transformation of a spherical representation with a matrix properly preserves the angular mask. |
This is in preparation for addings masks, so that if a SphericalDistribution has a separate mask on the radial coordinate, that will be propagated to the RadialDistribution.
…e-spot combo. With the option to get a combination of the masks of selected components.
72c178b to
ca5126c
Compare
|
@eerovaher - @nstarman already approved, but I'd rather have your OK too before I merge... Will add that I'm sorry that my splitting between representations and frames has made it more difficult instead of simpler (as I thought it would be -- the representation case is much simpler and I thought it would have gone in quickly so we could concentrate on the frames...). While I'm not sure I have addressed all your concerns on the frames, though, I think I did get everything on the representations. Thanks! |
|
Dividing work into smaller self-contained chunks is a good idea, but (as I've said multiple times already) I don't see why we shouldn't be able to merge #17016 before the feature freeze. |
|
OK, that's fine too! |
This pull request is the first step in making
SkyCoordable to deal with masks, by giving representations a mask. In general, I tried to make the behaviour similar to that ofTime, which means, e.g., that the mask strictly refers to that of the data, not any associated differentials (similarly, for frames, themaskwill not include information from any attributes, just likeTimedoes not include information fromlocation).One difference is that the mask is not shared between the three components. This is because I think we want to be able to support
SkyCoordof catalogues in which some of the sources do not have an associated distance. Most logical is for those just to have a masked distance, but unmasked longitude and latitude. But to support that, the components ofSphericalRepresentationneed to have independent masks. (I went some way in seeing if the angles still could have a shared mask, but ended up feeling this became too complex and fragile)So, right now, the representations have a
.maskproperty which just or's everything together, as well as aget_maskmethod, which allows one to select specific components.EDIT: This is a follow up of #16844
Note that this builds on #16842 and #16844. The most important commit is the last one. Opening as draft for now to allow comments on the general idea first.