Adjust RotatedSunFrame to rely on supported Astropy machinery - #4691

Thanks for the PR @adrn! 👍

You are correct that – unlike SkyOffsetFrame – it is essential for RotatedSunFrame to preserve the component names, so your PR here is on the right track for the modifications needed for RotatedSunFrame to subclass BaseCoordinateFrame. It looks like there may be some wonkiness because it's now picking up an obstime frame attribute, which is different behavior. That may turn out to be an easy fix to remove, or that may require some re-jiggering of the RotatedSunFrame API to accept it.

But, I'd like to understand why RotatedSunFrame ought to be a subclass of BaseCoordinateFrame. (It would have avoided the bug in astropy/astropy#11096, but that's moot now.) In a spiritual sense, RotatedSunFrame does not behave at all like a BaseCoordinateFrame: you never have an instance of the class itself, you can't specify the class itself as the frame= for SkyCoord, it's not a valid node in the transform graph, etc. Importantly, I'd argue that it's even useful for issubclass(RotatedSunFrame, BaseCoordinateFrame) to be False because otherwise you might be able to pass RotatedSunFrame past some checks that it really shouldn't pass.

The frame class it creates dynamically (e.g., RotatedSunHeliographicStonyhurst) will subclass from BaseCoordinateFrame regardless (e.g., RotatedSunHeliographicStonyhurst subclasses the normal coordinate frame HeliographicStonyhurst in addition to RotatedSunFrame). I certainly may be missing something, but I don't see the reason/value in making this change. The only part in the existing implementation that may raise an eyebrow is the specification of frame attributes (base, duration, rotation_model) for something that isn't a subclass of BaseCoordinateFrame, but of course they end up being frame attributes for the created frame class.

Pending discussion, I'm changing this PR to draft status.

I've also changed the labels since it's unavoidably an API change (formally speaking) due to a different class inheritance, and I worry that getting the frame attributes sorted out will ripple out to more disruptive changes to the API.

Hmm, upon further thought, I'm not sure why the current implementation of RotatedSunFrame isn't registered to have the frame attributes (e.g., obstime) from the "base" coordinate frame (e.g., HeliographicStonyhurst). I may be relying on some internal, possibly unintended, behavior in the Astropy machinery...

I've also changed the labels since it's unavoidably an API change (formally speaking) due to a different class inheritance, and I worry that getting the frame attributes sorted out will ripple out to more disruptive changes to the API.

How should we deal with this on the LTS?

2.0 will work again once astropy/astropy#11101 is merged. This PR isn't necessary for 2.0 (as far as I know).

The only part in the existing implementation that may raise an eyebrow is the specification of frame attributes (base, duration, rotation_model) for something that isn't a subclass of BaseCoordinateFrame, but of course they end up being frame attributes for the created frame class.

The about-to-be-merged astropy/astropy#11099 updates the descriptor implementation in such a way that this "hack" no longer works, so something definitely needs to be fixed with RotatedSunFrame.

Amusingly, astropy/astropy#11099 also "fixes" the problem with this PR where RotatedSunHeliographicStonyhurst gains the obstime frame attribute, in that it results in bugged behavior of not registering that frame attribute.

@adrn I've pushed some significant changes to this PR. Since the design intent of, e.g., RotatedSunHeliographicStonyhurst is that it is treated differently from HeliographicStonyhurst and doesn't have any of its frame attributes, I have removed that subclassing relationship.

Kicking Azure

The PR now preserves all API associated with RotatedSunFrame aside from the following class inheritance:

• RotatedSunFrame now subclasses SunPyBaseCoordinateFrame. Previously, it subclassed nothing (okay, object).
• RotatedSunHeliographicStonyhurst now subclasses only RotatedSunFrame. Previously, it subclassed HeliographicStonyhurst in addition to RotatedSunFrame. (HeliographicStonyhurst is just a representative example, of course.)

Given the niche impacts of these inheritance changes, I think this PR is acceptable to backport to 2.0.

Kicking RTD

The backport to 2.0 failed:

Commits ["99bb3c9db151b9ad8fb735a82ce8c69ba03be3eb","ac6fd83c8aedb7f01e0761f19bcdc0e4fe42aedf","d263dc03b998849601b82b3f539b9b9631385c55"] could not be cherry-picked on top of 2.0

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub.
git fetch
# Create new working tree.
git worktree add .worktrees/backport 2.0
# Navigate to the new directory.
cd .worktrees/backport
# Cherry-pick all the commits of this pull request and resolve the likely conflicts.
git cherry-pick 99bb3c9db151b9ad8fb735a82ce8c69ba03be3eb ac6fd83c8aedb7f01e0761f19bcdc0e4fe42aedf d263dc03b998849601b82b3f539b9b9631385c55
# Create a new branch with these backported commits.
git checkout -b backport-4691-to-2.0
# Push it to GitHub.
git push --set-upstream origin backport-4691-to-2.0
# Go back to the original working tree.
cd ../..
# Delete the working tree.
git worktree remove .worktrees/backport

Then, create a pull request where the base branch is 2.0 and the compare/head branch is backport-4691-to-2.0.