Improve SkyCoord and Frame equality operators - #20211

Short version: the dispatch skeleton is identical and the four leaf comparisons
all differ. The repetition is in the type-dispatch, not in the logic that
matters.

Block 1 — signature: identical

@staticmethod
def _frameattr_equiv(left_fattr, right_fattr):          # noqa: PLR0911
def _frameattr_eq_elementwise(left_fattr, right_fattr): # noqa: PLR0911

Both are staticmethods taking two raw attribute values, and both trip ruff's
"too many return statements" rule for the same reason: they are dispatch tables
written as a chain of early returns.

Block 2 — identity and None shortcuts: identical

if left_fattr is right_fattr:
    return True
elif left_fattr is None or right_fattr is None:
    return False

Byte-for-byte the same (the new one has an extra clause in the comment noting
that the first branch also covers both being None).

Worth knowing how load-bearing that first line is: because Attribute.__get__
caches the broadcast value back onto the instance, two frames sharing an
attribute object hit the is shortcut and never reach a real comparison. That
is why several array-valued-frame-attribute tests passed before this work even
though the element-wise path did not exist yet — they compared a Time object
against itself.

Note this shortcut is also where both helpers return a plain scalar True
regardless of the attribute's shape. For the element-wise helper that is fine:
the caller &s the result into the data comparison, which carries the shape.

Block 3 — representation type-mismatch guard: identical

left_is_repr = isinstance(left_fattr, r.BaseRepresentationOrDifferential)
if left_is_repr ^ isinstance(right_fattr, r.BaseRepresentationOrDifferential):
    return False

Identical. A representation compared against a non-representation is False
with no element-wise nuance available, so there is nothing to change.

Block 4 — representation body: substantially different

Old:

if getattr(left_fattr, "differentials", False) or getattr(right_fattr, "differentials", False):
    warnings.warn("... at least one of them has differentials.  This yields False "
                  "even if the underlying representations are equivalent ...", AstropyWarning)
    return False
return np.all(
    left_fattr == right_fattr
    if type(left_fattr) is type(right_fattr)
    else left_fattr.to_cartesian() == right_fattr.to_cartesian()
)

New:

left_diffs = getattr(left_fattr, "differentials", {})
right_diffs = getattr(right_fattr, "differentials", {})
if left_diffs.keys() != right_diffs.keys() or any(
    type(left_diffs[key]) is not type(right_diffs[key]) for key in left_diffs
):
    return False

if type(left_fattr) is not type(right_fattr):
    if left_diffs or any(isinstance(fattr, r.BaseDifferential)
                         for fattr in (left_fattr, right_fattr)):
        return False
    left_fattr = left_fattr.to_cartesian()
    right_fattr = right_fattr.to_cartesian()

return left_fattr == right_fattr

This is the block that actually diverges, and it is the only semantic change
between the two helpers (see §10.1). Four differences:

1. The differentials bail-out is gone. The old code refuses to compare as
   soon as either side has differentials, warns, and returns False — its own
   warning text admits this is wrong ("this may change in future versions"). The
   new code compares them, which is sub-decision 5 from §6.
2. A narrower guard replaces it. BaseRepresentation.__eq__ raises
   ValueError on mismatched differential keys and TypeError on mismatched
   differential classes, so those two cases are checked up front and return
   False rather than being caught after the fact. This is preferred over a
   try/except so that a genuine shape-mismatch ValueError still propagates.
3. The cross-class conversion became a statement instead of a conditional
   expression, because it now needs the extra guard: to_cartesian() silently
   drops differentials, and on a BaseDifferential it requires a base argument
   that is not available here. The old one-liner would raise TypeError on
   cross-class differentials — latent, since no built-in frame has an attribute
   that can hit it.
4. No np.all(). The point of the exercise.

Block 5 — coordinate type-mismatch guard: identical

left_is_coord = isinstance(left_fattr, BaseCoordinateFrame)
if left_is_coord ^ isinstance(right_fattr, BaseCoordinateFrame):
    return False

Identical, same reasoning as block 3.

Block 6 — coordinate body: different, but converging

Old:

return left_fattr.is_equivalent_frame(right_fattr) and np.all(left_fattr == right_fattr)

New:

if left_fattr.__class__ is not right_fattr.__class__:
    return False
if left_fattr.has_data != right_fattr.has_data:
    return False
return left_fattr == right_fattr

Both exist for the same reason, stated in the old docstring: comparing
coordinates "first checks whether they themselves are in equivalent frames
before checking for equality in the normal fashion. This is because checking for
equality with non-equivalent frames raises an error."

The new version guards the same hazard, but the hazard shrank. After this PR,
frame == only raises on a class mismatch or a data/no-data mismatch, so those
are exactly the two conditions checked. Everything else recurses into the
element-wise frame comparison — which is the point, since a CoordinateAttribute
should get the same element-wise treatment as any other coordinate.

Using is_equivalent_frame here would have been wrong for the new helper: it
would collapse a nested attribute difference back to a scalar False. This is
the deviation from the original plan noted in §9.

Block 7 — fallback: differs only by np.all()

return np.all(left_fattr == right_fattr)   # old
return left_fattr == right_fattr           # new

This is the path taken by Time, Quantity and EarthLocation attributes —
i.e. almost every real frame attribute. Here the two helpers really are the same
code modulo the collapse.

10.1 Could they be merged?

Nearly. Running both over 25 representative attribute pairs and comparing
np.all(_frameattr_eq_elementwise(a, b)) against _frameattr_equiv(a, b), they
agree on 24 (script: compare_helpers.py). Only two cases diverge, and neither
is a case where the old behavior is defensible:

The first is the intentional improvement (sub-decision 5). The second is a
pre-existing bug in _frameattr_equiv, and therefore in
is_equivalent_frame, which is documented as returning bool and raising only
TypeError. It is reachable from the public API, because
CoordinateAttribute.convert_input passes a value through untouched when it is
already an instance of the right frame class, data or not:

g1 = Galactocentric(..., galcen_coord=ICRS(1*u.deg, 2*u.deg))
g2 = Galactocentric(..., galcen_coord=ICRS())          # no data, accepted
g1.is_equivalent_frame(g2)
# ValueError: cannot compare: one frame has data and the other does not

So _frameattr_equiv could in principle become:

@staticmethod
def _frameattr_equiv(left_fattr, right_fattr):
    return bool(np.all(BaseCoordinateFrame._frameattr_eq_elementwise(left_fattr, right_fattr)))

deleting ~35 lines and the duplicated dispatch entirely. That was not done
here, deliberately:

• It would change is_equivalent_frame, and through it __setitem__,
  concatenate, spherical_offsets_to and transform loopback detection — frames
  whose representation attributes carry equal differentials would become
  "equivalent" where they previously were not. That is almost certainly the
  correct behavior, but it is a second API change with a much wider blast radius
  than the one this PR is making, and it deserves its own review.
• The ValueError fix is a genuine bugfix that stands on its own and would be
  easier to review as a separate PR against main.

Recommendation: land this PR as-is with the duplication, then follow up with a
PR that fixes the is_equivalent_frame ValueError and collapses
_frameattr_equiv into a np.all() wrapper. Splitting it that way keeps the
is_equivalent_frame semantic change reviewable on its own terms rather than
riding along inside an equality change.