Skycoord attributes slicing, dicing, propagation - #5751

Do you need some guard against users changing, e.g.

attr = frame_transform_graph.frame_attributes
attr['ra'] = 'something that changes the original frame_attributes'

? Or will the documentation say 'Don't do that'.

@dmopalmer - perhaps I should make this a private attribute, indeed, possibly exposing just the keys (as your frame_attrnames_set would have done)

I updated this following the discussion in #5750, which concluded that it would be good if non-current frame attributes could be modified on SkyCoord (since this was already used in other packages).

With that, this PR really just fixes bugs, with the very nice speed-up from #5703 as a nice side benefit.

A remaining question is whether frame_attributes should be a public or a private propery on frame_transfer_graph.

@mhvk - I've been in a busy patch but this is very much on my radar to review.

Small update: Since we allow random setting, I thought I also should allow deleting extra attributes.

This brings up another small question about public vs private: should _extra_attr_names become a public attribute? It may be good for introspection for code to be able to know which extra attributes have been set.

Anyway, @taldcroft - no hurry -- I have quite a few PRs that I should review myself (sorry, @dmopalmer!).

I haven't yet reviewed this in full detail, but a couple high-level things I'm concerned about, @mhvk:

1. I'm not sure it's fair to call this a "bug fix"... this is a pretty substantial change in how things work. I'm open to the argument, but my first reaction is that this should be in 2.0?
2. I'm worried about the promise of always validating is locking us into some things we may not want to do. In particular, if a user creates a new frame with a name that overlaps with an existing frame, what's the right validator? Right now we side-step this by saying you don't validate until the frame is reached... But this changes that. I think this also has optimization implications because it makes it impossible to do some of the planned speedups that might be possible by simplifying the FrameTransformGraph's frame attribute machinery...

@eteq - quick answers:

1. While I don't particularly mind the milestone, this PR does fix two particular bugs: Skycoord __getitem__ not being applied to frame attributes #5743, SkyCoord(frame=other.frame) doesn't copy all frame attributes #5749; it is very hard to see how to fix these without something like what I have done here.
2. I did think about validation at a later stage, but it is quite a bit less handy. In particular, while validation when transforming to a new frame could be done in the initialiser, this doesn't address slicing. As it is, it is a very easy change to __getitem__ to allow the extra attributes to be sliced, but the whole machinery with _apply relies on things already being in sliceable form -- I don't think it would be a good idea to add validation there (yet we have to be able to slice them otherwise (azalt.transform_to('icrs')[0].transform_to('altaz') != azalt[0].

I do see your argument about users defining new frames with overlapping names (I'm not worried so much about existing names -- which can be found even by tab-completion -- but I could imagine someone defining one that later we would define separately), but do not really see how to work around that. In this respect, cc @Cadair since sunpy has defined new coordinate frames.

And of course a major advantage is that users now get the error when they set the attribute, rather than get it only when transforming to something else!

As for speed, my own sense is that this will actually help speed up things: if we know all attributes are validated, there is really no reason any more to go through full initialization upon slicing and transforming (for representations, this makes quite a speed difference; first attempts in #5558).

p.s. I rebased now that #5750 is in, so that we can discuss just what's new here.

p.p.s. I looked through this again, and still think it is a good approach. Note also that it speeds up things already, mostly because of @dmopalmer's caching of all frame attributes, but also because we don't set any extra frame attributes unless they are actually passed in (before all possible attributes were set explicitly to None).

@eteq - now that you are in coordinate land, would you be able to have a look at this? I think it is a fairly substantial improvement, and as it fixes two bugs, still feel it belongs in 1.3.1.

The travis failure can be ignored: URL timeout (what is new...)

@eteq - could you have a look at this, so I can get 1.3.1 out?

@mhvk, one clarification: in what way does this fix #5749 ? I was trying to build a regression test to understand better what the larger side-effects are (critical for the 1.3.x vs 2.0 call), but I don't see any change to the example case from #5749 (which actually I think is good ... but want to make sure I'm not missing something)

Indeed, #5749 should not change. What this fixes is slicing of attributes on SkyCoord (#5743), your original #5022. #5743 is I think a real bug, but I can see how your hesitant with this big a change in a bug-fix release. I'll leave that up to you; I mostly want to have this merged so we get to a good state for the velocity work...

w.r.t coordinate name overlap we did nearly have that when astropy added the heliocentric (ICRS) frame and it in the initial PR was called HelioCentric which is the name of one of the sunpy frames. I doubt it's very likely to be a major problem though.

Alright, I'm just not comfortable introducing this as a bugfix (even if it does fix a bug) because of some of the subtle consequences. If nothing else, the fact that @dmopalmer has likely worked around it for #5743 suggests there are people "in the wild" who are doing things assuming the current behavior.

So I'm going to milestone this 2.0. But I think I'm convinced that this should go in, just need to finish some additional test cases (and a final review). Hopefully will do that later today.

@eteq - Fair enough. More tests are definitely a good idea!

@eteq - I agree that your test should work, but after spending the better part of 5 hours on it, the conclusion is that it will need a rather complete overhaul of _parse_inputs and its helper routines such as _get_frame. This made me wonder if the tests ever worked, and it turns out the answer is no, the test you added fails on current master as well as on 1.3 and on 1.0.12. So, I think the right thing to do is to raise a new issue for this test (#5882), and remove it from this PR (which I have now done). With that, this PR can probably go in (assuming I didn't mess something up and tests still pass).

p.s. I did get reasonable far on the input parsing, so will try to solve #5882 soon(ish).

Oh, good point @mhvk! Agreed this is a separate issue, then. Will merge this now.