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.

• Do the proposed changes actually accomplish desired goals?
• Do the proposed changes follow the Astropy coding guidelines?
• Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
• Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
• Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
• Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
• Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
• Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
• At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

Detailed explanation of _frameattr_equiv vs. _frameattr_eq_elementwise

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.

In astropy/astropy-project#538 (comment), @astrofrog commented that this is an example where PRs should be split into smaller parts where possible.

I strongly agree with the principle. In this particular case I'm not so sure because one of the key outcomes of this PR is unifying the behavior for equality of SkyCoord and Frame. I certainly could (and am willing) to split this into two PRs:

1. Improve BaseCoordinateFrame equality.
2. Improve SkyCoord and unify with BaseCoordinateFrame.

The hitch here is first that (1) is not hugely valuable on its own. Another hitch is that both of these are API changes that would merit a What's New, so (1) would provide an intermediate version and then (2) the final version. Because these changes are intrinsically coupled I'm not convinced that splitting into two PR's will reduce the overall level of effort.

Can you give us a feeling for how urgent this is?

I haven't had time to provide a detailed review of the pull request yet, but from a first glance much of the complexity seems to arise from how BaseCoordinateFrame handles equality and much of that code is liable to change given the work towards APE 23.

Since that APE is supposed to be stripping data out of the frame classes, it arguably makes much of this work redundant. If its not urgent, perhaps we can delay until the APE23 work is done. Although perhaps there is still merit in hashing out what equality for SkyCoordinate object should mean now...

@StuartLittlefair - thanks for the response. There is no particular urgency on this PR. It basically stems from a long-standing (somewhat low priority) issue that grouping a Table by a SkyCoord object doesn't work because SkyCoord equality is a bit broken. So I stepped out of my lane to take a crack at fixing it.

I'm happy enough to put a hold on this PR or leave it as a demonstration of what could be done. I might see what it would look like to make an isolated change purely in SkyCoord that defines equality as all the attributes (frame attributes and "extra" attributes) and data being equal in a broadcasted sense. If there is any discussion related to defining equality I'd be happy see it.