SkyCoord is_equivalent_frame behaves oddly

[alexrudy]

I expected that is_equivalent_frame() should return true if I e.g. convert a whole bunch of sky coords to 'icrs', but the following test fails on master.

def test_is_equivalent_frame():
        """Test if frames are equivalent"""
        sc_fk5 = SkyCoord(1*u.deg, 2*u.deg, equinox='J2000', frame='fk5')
        sc_icrs = SkyCoord(1*u.deg, 2*u.deg, frame='icrs')

        sc_after1 = sc_fk5.transform_to('icrs')
        assert sc_after1.is_equivalent_frame(sc_icrs)

In this example, note that

sc_after1.equinox == Time("J2000")

which is why is_equivalent_frame() fails.

This came up when I was trying to turn a list of SkyCoord objects into a single SkyCoord object, using something like SkyCoord([ c.transform_to('icrs') for c in my_coords])

[eteq]

@alexrudy - this is actually intended behavior, if counter-intuitive. If you look in the SkyCoord.is_equivalent_frame docs, you'll see this:

For two `SkyCoord` objects, *all* of the frame attributes have to match, 
not just those relevant for the object's frame.

The reason for this is a bit subtle: SkyCoord objects keep a memory of frame attributes even if they aren't relevant for the frame they are currently in, because what if you wanted to do this:

sc_after2 = sc_after1.transform_to('fk5')

it would be very surprising to find that sc_after2 is not the same as sc_fk5, but it would be for any equinox other than J2000 (the default for FK5). So SkyCoord's have to know their "historical" frame attributes. So then SkyCoord.is_equivalent_frame gives False if all of that doesn't match.

To get the behavior I think you're looking for, you could replace your assert instead with:

assert sc_after1.frame.is_equivalent_frame(sc_icrs.frame)

which checks the frame objections themselves, and they have no knowledge of the extra SkyCoord frame attributes.

[eteq]

If that makes sense, @alexrudy, I think you can go ahead and close this, as I don't think it's a bug. But if you have some thoughts about how we might make this clearer in the docs or something, feel free to keep this open to remind us to implement those ideas! (or PR youself if you have a firm enough idea)

[alexrudy]

@eteq this makes sense if we are just using is_equivalent_frame as a user, but I hit this bug when doing something like this:

SkyCoord([ c.transform_to('icrs') for c in my_coords])

and I'm not sure why that pattern shouldn't work or be allowed? Although it is nice to be able to transform back to the same frame with the same attributes, I do think that two coordinates that I have explicitly transformed to ICRS should be in equivalent frames?

[adrn]

@alexrudy Could you provide some more context for the code in the above example, or ideally a snippet that reproduces the error?

Also, I'm curious why you wouldn't just do my_coords.transform_to('icrs') in the above?

[alexrudy]

@adrn I get a list of coordinates (from a user's starlist, for e.g. an observing run) which might be in different frames. But I want to use a catalog matching function, so I make them into a single sky coordinate in the same frame, usually 'icrs' because thats a good reference frame. But I can't just turn the list of SkyCoord objects into a single vector SkyCoord without first transforming them to the same frame. my_coords in this case is just a regular python list of several SkyCoord objects.

[StuartLittlefair]

@eteq can you think of another reason why SkyCoord should remember historical attributes? I can't understand why it wouldn't be better to simply set the equinox attribute of sc_after2 in your example.

The conversion case posted by @alexrudy is a clear example of how the "hidden" storage of non relevant attributes leads to odd behaviour.

[StuartLittlefair]

@alexrudy - BTW, your use case has other issues. For example, what if some coords in your list have distances and some do not?

See https://github.com/astropy/astroplan/pull/215/files for an example of how I addressed this problem.

[alexrudy]

@StuartLittlefair Agreed, my method has issues if there are distances attached. Fortunately, my starlist format doesn't permit distances, so I'm fairly certain I'll end up with all-unitspherical coordinates. I'm happy for my method to raise an error in that case anyways. Or even better IMO would be if SkyCoord would accept something like:

SkyCoord([co1, co2, co3], frame='icrs')

and then it could raise an appropriate error if it can't convert everything to the same frame.

[alexrudy]

I'm not sure how to proceed here. Although I think holding on to attributes is a nice feature for SkyCoord so that coordinates can round trip, I'm not sure about a few things:

• Is there a reason I can't convert multiple sky coordinate objects to the same frame and then make an array of them?
• Why is this helpful round-tripping behavior only applied to SkyCoordinates and not Frames? I know SkyCoordinate is supposed to be "helpful", but this does seem to change the behavior in an odd and unexpected way (at least in my use case). I could see retaining the frame attributes always, but forcing the user to understand "is equivalent frame" and "is identical frame with identical attributes" as two different things. It seems like we mean the first when applied to a frame object, and the second when applied to a SkyCoord, even though the methods have the same name.
• What should be done to convert say an ICRS coordinate and an FK5 coordinate to the same frame, and make a vector of SkyCoordinates?

@StuartLittlefair's referenced solution is fine, but really clunky for something that otherwise has a pretty nice high-level API.

[alexrudy]

I'd like to poke at this again. If we can agree on how we expect the API to work, I'm happy to write the pull request to do so, but I feel like this discussion is stalling.

[adrn]

Just adding a note that this and #3182 should probably be combined once we reach consensus.