[#11094] Deprecate and remove use of OrderedDescriptor - #11099

Looks like I forgot to update my master branch because I didn't have #11090 incorporated yet. I'll rebase.

Great - thanks @embray! Agreed that this looks good and just requires a changelog entry.

Not a huge deal, but I believe this PR results in incorrect behavior when a frame class subclasses two different subclasses of BaseCoordinateFrame. I'll try to craft a minimal example.

Here you go:

>>> from astropy.coordinates import BaseCoordinateFrame, Attribute

>>> class Frame1(BaseCoordinateFrame):
>>>     attr1 = Attribute()
>>> class Frame2(BaseCoordinateFrame):
>>>     attr2 = Attribute()

>>> class Frame3(Frame1, Frame2):
>>>     pass
>>> Frame3.frame_attributes
{'attr1': <astropy.coordinates.attributes.Attribute at 0x1bfded96248>}

Note that attr2 is not picked up. I presume this is because __init_subclass__() – which is intended to accumulate all of the frame attributes – isn't actually called multiple times even though there are multiple subclasses.

For comparison, on master:

>>> Frame3.frame_attributes
OrderedDict([('attr1',
              <astropy.coordinates.attributes.Attribute at 0x29bb9e74348>),
             ('attr2',
              <astropy.coordinates.attributes.Attribute at 0x29bbad9dbc8>)])

(Oops, accidentally pushed commits for a different branch)

Thanks for the example @ayshih. I had thought about this case, but since it didn't happen to come up in any of the Astropy tests I assumed it maybe wasn't a usecase that was needed. Maybe it just wasn't tested because this case was tested implicitly by the use of OrderedDescriptor. But I completely agree with you on how this should work. I'll fix it and add a test.

Out of curiosity @ayshih, what would be the use case of subclassing two different frames?

(Also, gh-11098 was merged, so this could be rebased)

@astrojuanlu thanks for the heads up, will do.

I'm also curious about @ayshih's use case of multiple inheritance of frames, but nevertheless they're right that it's a regression over the previous version that should be fixed. I'll try to finish that now.

Rebased and fixed the multiple inheritance case.

Fixed the style check and added changelog entry.

In the changelog I referenced the original issue number, not the PR number. Is that what we're doing now?

There are failures in modeling though.

I don't think those failures are caused by this PR!

You are right! Seems to be from #11108 that just got merged before this was rebased. Or due to upstream changes and the timing is coincidental.

Wait... Ugh, I think it is due to Scipy 1.6.0 that just got released. I'll submit a stop gap PR (see #11201).

Just a 🎆 from the sidelines (will light some real ones later...)

@astrojuanlu

Out of curiosity @ayshih, what would be the use case of subclassing two different frames?

@embray

I'm also curious about @ayshih's use case of multiple inheritance of frames

In principle, this could be useful for using class inheritance to manage common frame attributes. For example, there could be a base class that adds the equinox frame attribute rather than defining that attribute separately within each frame definition (and possibly introducing an inconsistency). It'd be particularly useful if there are any convenience methods that'd be useful to any frame with an equinox frame attribute, but not useful to any other frame.

In practice, Astropy already has cases of subclassing two different frames, although this may be a design oversight. Any frame created by SkyOffsetFrame subclasses both SkyOffsetFrame (which has the rotation and origin frame attributes) and the frame of the origin argument (which has its normal frame attributes, e.g., obstime). For example:

>>> import astropy.units as u
>>> from astropy.coordinates import SkyCoord, SkyOffsetFrame

>>> sc = SkyCoord(0*u.deg, 0*u.deg, frame='hcrs')
>>> so = SkyOffsetFrame(origin=sc)
>>> so
<SkyOffsetHCRS Frame (obstime=J2000.000, rotation=0.0 deg, origin=<HCRS Coordinate (obstime=J2000.000): (ra, dec) in deg
    (0., 0.)>)>

>>> so.__class__.__bases__
(astropy.coordinates.builtin_frames.skyoffset.SkyOffsetFrame,
 astropy.coordinates.builtin_frames.hcrs.HCRS)
>>> so.frame_attributes
OrderedDict([('obstime',
              <astropy.coordinates.attributes.TimeAttribute at 0x18959ce1e88>),
             ('rotation',
              <astropy.coordinates.attributes.QuantityAttribute at 0x18959cfb708>),
             ('origin',
              <astropy.coordinates.attributes.CoordinateAttribute at 0x18959df5b88>)])

Re SkyOffsetFrame, I say "design oversight" because for a similar frame in SunPy (RotatedSunFrame), I have recently decided to remove the inheritance of the frame class of the supplied coordinate. SkyOffsetFrame may not be able to make the same decision, of course.

Fixed changelog conflict.

OK, just checked again and this all looks great. Let's get it in!