Transforming a frame to a class now preserves shared frame attributes - #10453

@ayshih - by way of background, for the SkyCoord transform part of the reasoning is that the coordinate even keeps attributes which are not on the current frame (say, temperature for ICRS). This, of course, cannot be done between frames.

I think the lack of auto-transfer of attributes was partially on purpose, since the idea was that frames are quite basic and one should just specify exactly what one wants. That said, it is not obvious to me this has to be this way... cc @eteq, @astropy/bots

I guess one question is whether there is any particular reason you want to work with the frames directly instead of with SkyCoord?

In the example I provided, I think it can be confusing to users that the hme coordinate points to Sun center at the explicitly provided obstime, but transforming to the HeliocentricTrueEcliptic class returns a coordinate that is no longer at Sun center because the obstime is now at the default value of J2000.0. Note that because a class is provided as an argument, the user never actually "sees" the default values in their input. I don't like that design choice.

Certainly my advice to SunPy users who've encountered undesired behavior with .transform_to() has been to be explicit about the target frame or to use SkyCoord (e.g., sunpy/sunpy#4237 (comment)). However, it keeps coming up because I don't think it's terribly clear to users what the difference is between using and not using SkyCoord when both approaches have a .transform_to() method that accepts the same input.

Personally, the behavior that I'd like to fix is in the internals of CoordinateAttribute, which transforms the input to the specified frame class (see

Admittedly, a custom CoordinateAttribute variant would solve the above. But that wouldn't address the user confusion I described earlier.

@ayshih - I agree it is confusing and I think it might well make more sense to have the frame transform_to also have a merge_attributes argument. That said, if the default is True, as for SkyCoord, it risks breaking people's code, yet if the default is False, at some level we keep the same problem... I think this will be a decision that needs more than one of the coordinate maintainers, so let me ping @StuartLittlefair, @adrn, @astrojuanlu, and @eteq.

For the CoordinateAttribute, is there also a problem if one sets it to a SkyCoord?

Hi @mhvk and @ayshih - I totally agree that the example provided is confusing. Also, I think that the behaviour enabled by this PR is slightly less confusing. But the crucial behaviour is still "hidden" and will catch some people out - so it's still a potential area of misuse.

I'm not sure I'm keen on introducing a change that will break someones code to move to a situation that's only slightly less bad.

I wonder if a more radical change here is actually better, and we should deprecate passing classes to transform_to - for frames only, not SkyCoord? It's in line with the idea that frames are kept as a lower level implementation that requires more work from the user, and is probably the least error-prone solution.

For the CoordinateAttribute, is there also a problem if one sets it to a SkyCoord?

In fact, even though CoordinateAttribute strips off any SkyCoord layer to always return just a frame, its internal .transform_to() call occurs before the SkyCoord layer is removed, so its converted coordinate will differ in frame attributes depending on whether SkyCoord is used. I don't know whether you'd consider that a bug or a feature. I'd argue that's even more confusing for users, given that the .transform_to() call is internal.

I'm not sure I'm keen on introducing a change that will break someones code to move to a situation that's only slightly less bad.

Fair enough!

I wonder if a more radical change here is actually better, and we should deprecate passing classes to transform_to - for frames only, not SkyCoord? It's in line with the idea that frames are kept as a lower level implementation that requires more work from the user, and is probably the least error-prone solution.

The more I consider this "radical change", the more I like it! It draws a brighter line between working with SkyCoord and working with just frames. I don't know how many places the code would need to be adjusted for such a change, but it many places, it could be as straightforward as just adding () to instantiate the class being passed to .transform_to().

In the larger picture, for frame1.transform_to(frame2), I agree it makes sense to think of frame2 as a frame instance, indeed, as an instance that does not have actual data (since the data is never used for the transformation). After quite a bit of pondering, I also agree that no longer allowing the class to be passed in is a nice solution. I particularly like that it can have a clean deprecation path with a warning that would explain that the frame transform_to strictly transforms to another frame, with all its attributes, and that users may want to use SkyCoord if that is not the intent. So, 👍 to this idea!

Note that I changed my mind twice, since just letting the class stand for the "default instance" seemed harmless. I do think @StuartLittlefair is right that to change the meaning to be "an instance with attributes like that of the original frame where possible", while better than "default instance", is too risky -- or rather, too late to change it. Overall, given my changes of mind, I guess we need to be sure to write this up carefully!

Actually, maybe this situation also points to a way to help make things clearer to the user, that a frame is really mostly meant as a way to describe the orientation of the reference system, with a bit more detail also on how the standard representations look like, but the fact that a frame holds the positional data is at some level an implementation detail [1]. Really, SkyCoord could internally hold a frame and representation and everything would work the same (well, in that case frame1.transform_to(frame2) would not really exist, but rather it would return a transformation function or would have a signature that included the representation). I think it may be useful to go through the documentation and look carefully at examples where a frame is instantiated with data to see whether that really is helpful, or whether one rather should go through SkyCoord.

In this respect, might it make sense for CoordinateAttribute to actually always hold a SkyCoord (transformed to the relevant frame)? It would make sense given that for this atttribute, there does have to be associated data. And of course it would mesh better with the name...

p.s. The above is beginning to look awfully like three related but separate issues...

[1] APE5 notes about frames: "While it may at first make sense to separate specifying frames from the actual representations in those frames, doing so would result in a large amount of code duplication, as each system would require two very similar classes for specifying a frame and for the objects containing the representations. Hence, here we combine them into a single class that is a pure frame if it does not have position data, but if it does, is actually a coordinate." (see also the discussion of alternatives; I'm not sure I 100% agree, but that obviously is water under the bridge)

the fact that a frame holds the positional data is at some level an implementation detail [1]

Thanks for pointing to the APE! I've never actually read it, and it's nice to appreciate the context of design choices.

p.s. The above is beginning to look awfully like three related but separate issues...

Are these the three issues you have in mind?

1. Deprecate being able to pass a class to BaseCoordinateFrame.transform_to(), and point users to SkyCoord.
2. Revise any documentation examples that instantiate frames directly with data to instead use SkyCoord, unless there is some strong reason to not use SkyCoord.
3. Modify CoordinateAttribute so that it uses SkyCoord consistently and transparently, possibly even changing the converted output, although that may not allow for a sensible deprecation path. (It may be better to instead create a completely new frame attribute (e.g., SkyCoordAttribute) and deprecate CoordinateAttribute.)

Yes, indeed, those three exactly!

For (3), one "solution" might be for input that is a frame to first make it a SkyCoord, then transform, and then take out the new frame. That way one effectively gets the merge_attributes=True behaviour. Though perhaps easier (and with less risk of regressions) is to warn for the cases that the input is not a SkyCoord or already an instance of the right frame class (i.e., deprecate the option to pass in an instance of a different frame class).

As for the output, it might be interesting to see if any tests break if we change it to SkyCoord (other than those that check it is a frame, of course!). But perhaps both that and a possible new class are making the issue larger than it needs to be.

I've created #10475 for the suggested changes, so all future discussion should go there. I'll go ahead and close this PR.