Finishing up CartesianRepresentationFrameAttribute - #5253

I think this makes sense. Whilst I avoided this in #5207, in retrospect I don't know why. In principle it adds more API changes but I think that almost all third-party uses of get_gcrs_posvel will be to access objects that can be used to initialise GCRS frames and that use case will still work. And it has the advantage of making the returned objects easier to deal with. :+1 from me.

This is slightly off topic, but the code here makes me wonder if there a reason that CartesianRepresentations don't have dot and cross product methods?

This is slightly off topic, but the code here makes me wonder if there a reason that CartesianRepresentations don't have dot and cross product methods?

Or why one cannot add different instances or multiply them with a quantity... I've been thinking about that as well, but wasn't sure whether it would be useful (beyond being neat); I guess here are some real use cases! (But for another PR... Maybe raise it as an issue for now?)

@eteq - are you OK with ignoring the slight risk of backward compatibility problems with obsgeoloc and obsgeovel? I need this for coordinate frame shape-changes (indeed, #5254 includes this), so it would be helpful to decide one way or the other.

Rebased. Also one small change: in comparing representations, I think __eq__ should return an array of bool with the same shape as the representation; it only matters that a given element is different, not which component of that element differs.

@eteq -- with the shape-changing PR depending on this, I'd really like to get this in, so that I don't have to keep rebasing. The only outstanding issue was whether or not to deprecate; my sense remains not to bother, in which case it is ready as is (especially now that @MSeifert04 provided further review). Any objections?

@mhvk - OK, I thought about the backwards compatibility question a bit (and had a quick discussion with @astrofrog) and concluded that you're probably right that the effort/payoff ratio for trying to have a deprecation period is probably not worth it. I tried to search for cases of obsgeoloc being used "in the wild" (i.e. on github) and only found a few small examples so I think this is mostly safe. That said, given that we're going to do without the deprecation warning, I think we do want to make it a bit clearer in the docs how to work around this change. I'll issue a PR against @mhvk's branch shortly with what I have in mind.

There's another backwards compatibility problem I'm realizing, though: the change to make BaseRepresentation.__eq__ return an array. I think this change might be sensible, but it is a substantial change, as previous versions amounted to self is other. The original justification for that was that asking for equality in coordinates can be a dicey business - machine-precision errors can lead to counter-intuitive results like this (which happen in this PR):

>>> sr = SphericalRepresentation(10*u.deg,15*u.deg, 10*u.kpc)
>>> c1 = sr.to_cartesian()
>>> c2 = c1.represent_as(SphericalRepresentation).to_cartesian()
>>> c1 == c2
False

and the original conclusion was "we should leave it to users to decide what they think equality means". Of course we could change that... But perhaps that's best done as its own PR? Or is it necessary to include it here?

Oh, and this absolutely can't go in 1.2.2 if it's going to make a substansive change like this without a deprecation warning. Similarly with the __eq__ change. So I re-milestoned it to 1.3. Is that going to be a problem? I admit I'm really not clear which PR's are dependent on this (and whether they are new features vs. bug fixes)

Oh, actually, I guess this is supposed to include a fix for 1.2 via #5207? Hmm... it's really quite drastically against policy to make API-breaking changes between major versions without a very compelling reason. Is there any chance to make just the astroplan-touching parts work without this.

@eteq - The bug fix was for allowing multi-D obsgeoloc and obsgeovel. I don't know that that was urgent enough that we really have to fix it in 1.2, so am fine with just leaving it for 1.3.

On __eq__: see the comment in the old PR I just left for @StuartLittlefair -- it may be fine to just not introduce it at all (or at least leave introduction to another PR, if we think it is useful; it might be OK as part of any follow up on #5255).

OK, I merged in the "what's new" change, moved other change-log entries to 1.3, and removed the ability to do comparisons between representations. I do think we eventually want to have that, but it seems better done in the context of #5255.

OK, cool, I think this is great, then. Travis is still running, but assuming it passes I think this is good to go!

I actually do see the value of the __eq__ changes - in retrospect I think this new implementation might be better. But it deserves a wider discussion given that it's a substantive compatibility change. So lets continue in #5255 (or maybe make a PR specifically with that change?)

OK, travis now passed, so merging! Thanks for the reviews, everyone!