Consider deprecating BaseCoordinateFrame.representation in favor of representation_cls

[adrn]

I've always found it confusing that frame.representation points to a class of the representation. I propose we deprecate this attribute in favor of frame.representation_cls.

I've already implemented the new attribute in #6219 (which supports and can handle either .representation or .representation_cls), but we should decide if I should put a deprecation warning in there for v2.0 or wait. My vote is that we put it in v2.0

Milestoned for 2.0 so we make a decision.

cc @eteq @mhvk @taldcroft

[bsipocz]

👍 for deprecating anything in 2.0, it will be much nicer to support the LTS if the deprecation is already in there.

[mhvk]

I agree that representation was awfully confusion - I'd definitely expect to get the current representation out. So 👍 on deprecating it, and also 👍 on putting the deprecation warning in 2.0. Indeed, that follows my suggested change in our deprecation policy!

[taldcroft]

👎 for doing this in a hurry without time for a considered discussion. While it is true that some people (mostly developers) might expect representation to return a Representation object, most casual users who know about this from the top-level API may think differently:

In [31]: sc = SkyCoord(1*u.deg,2*u.deg, representation='spherical')

In [32]: sc = SkyCoord(1*u.deg,2*u.deg, representation=SphericalRepresentation)

In [33]: sc.representation
Out[33]: astropy.coordinates.representation.SphericalRepresentation

In [34]: sc.representation = CartesianRepresentation

In [35]: sc.representation
Out[35]: astropy.coordinates.representation.CartesianRepresentation

In [36]: sc
Out[36]: 
<SkyCoord (ICRS): (x, y, z) [dimensionless]
    ( 0.99923861,  0.01744177,  0.0348995)>

In [37]: sc.data
Out[37]: 
<UnitSphericalRepresentation (lon, lat) in deg
    ( 1.,  2.)>

Agreed that the API is not perfect -- we have a representation object which is named data, but in fact this was a conscious decision from very early for practicality instead of purity because "data" is shorter and more intuitive for non-experts. But I think the API is not terrible.

Having written this out I would need a lot more convincing for this API change. We really cannot take such changes lightly as each one is a PITA for our community.

[eteq]

I'm 👍 for deprecating this in 2.0.

I have never encountered anyone who actually uses .representation who didn't say "what? that's really strange - why is it the class and not the object?" . I do agree with the point that .data is a better name, so we definitely don't want to change .data to .representation. But it is confusing that .representation exists and is not the representation itself. So I think the change to have representation_cls be the primary name is an immense help.

More generally, I think most people who've actually used this "in the wild" have probably spent enough time thinking about it that they understand why this is needed and can update. This is the sort of 95% (maybe 99%?) power-users... but it's a premium here on making this change because the coordinates stuff is (intrinsically, not just the code) so darn complex.

That said, I am sympathetic with @taldcroft's

We really cannot take such changes lightly as each one is a PITA for our community.

what I don't see is the downside in deprecating. I am not averse to deprecating this and then leaving it in for a longer time than the usual deprecation cycle... Or making a more active effort than we usually do to ensure this isn't a problem for folks... Or even saying "deprecated, but with no clear removal date"... That makes it clear that it should not be used in the future but we might still not need to break old code.

[mhvk]

@adrn - as I understand it, your suggestion is also to deprecate the representation argument in the initializer, and make that representation_cls as well, correct? I do note that, as written, the docstring for frames like ICRS states that the parameters are:

    Parameters
    ----------
    representation : `BaseRepresentation` or None
        A representation object or None to have no data (or use the other keywords)
    ra : `Angle`, optional, must be keyword
        The RA for this object (``dec`` must also be given and ``representation``
        must be None).
    dec : `Angle`, optional, must be keyword
        The Declination for this object (``ra`` must also be given and
        ``representation`` must be None).
    distance : `~astropy.units.Quantity`, optional, must be keyword
        The Distance for this object along the line-of-sight.
        (``representation`` must be None).
    copy : bool, optional
        If `True` (default), make copies of the input coordinate arrays.
        Can only be passed in as a keyword argument.

So, the instruction to the user is actually that representation includes data, which to me seems logical. Indeed, this is partially why I stated that the use of the attribute representation to just give the class was rather confusing.

p.s. In SkyCoord, the argument is a class or string. It probably should also be possible to give it as an instance (if it is not already).

[taldcroft]

Or making a more active effort than we usually do to ensure this isn't a problem for folks...

A good start would be sending email to astropy-dev with an complete description of the API change and allowing at least a week for commentary. I really think that introducing this idea a day after the 2.0 branching is the wrong thing.

Question - will this remove the ability to set representation with a string? We discussed this a long time ago and people like Mike Droetboom were quite supportive of using strings, i.e. not having to import a class in order to set something. This would lead you to the strange idiom of setting sc.representation_cls='cartesian'.

Sorry for putting up resistance, but this is a very visible part of the API that has been stable for nearly 3 years.

[Cadair]

I think I agree with @taldcroft on this, that deprecating this for 2.0 is too soon. It would require quite a few changes in SunPy to prevent our users being hit with a lot of deprecation warnings when they create any of our frames.

I think I am more in favour of keeping the representation attribute as it is at the moment, and allowing setting by strings as well. It confuses me sometimes as to where you can use string aliases for frames and representations and where you can't.

[mhvk]

Definitely agree that setting with strings should continue to be possible; I don't like having to import those representation classes unnecessarily. Could we have the best of both worlds and continue to allow setting .representation with a string or class, as well as passing string or class in for an argument? Since you already set with a string and get a class back, it is perhaps less of an API break to now get an instance back.
Possibly we then wouldn't even need representation_cls.

@Cadair - just to be explicit: sunpy users would be hit with warnings because it uses the setting of representation with a string/class internally, right? I think that does strongly argue for at most having a pending deprecation warning, which is not visible by default.

@adrn - It seems to me the answer is indeed that we should wait. This probably even more now that your velocity work has shown real short-comings of the current mechanism -- which I don't think we quite know how to solve yet. This suggests it is better to shake that out first, to ensure that any changes are useful.

p.s. Somewhat relatedly, I've gotten some ways in refactoring the SkyCoord initializer (mostly to speed it up), and it may well be that that will inform also what is handiest there.

[Cadair]

@mhvk Yeah we set .representation inside the frame constructor, so it would trigger the warning unless we had an explicit check for astropy 2.0.

[adrn]

For what it's worth, I was just suggesting representation -> representation_cls. I've had the same experience as @eteq with users (including myself) being quite confused that it's a class and not the instance itself.

Since making this issue, I've found a number of other problematic API features that we'll probably want to break (for v3.0) to make the differentials/velocity support cleaner and more natural (there is some discussion in #6219). So I agree that we should wait until we know the full scope of proposed changes, and make sure we're certain about the changes.

Re-milestoning to v3.0.

[eteq]

@adrn - as I understand it, your suggestion is also to deprecate the representation argument in the initializer, and make that representation_cls as well, correct? I do note that, as written, the docstring for frames like ICRS states that the parameters are:

Actually, this is about the property, not the initializer. That's why all of this is so confusing: in the initializer representation is often the actual representation itself, rather than the class. And that's good. But the representation attribute is a class, not a representation object.

All that said, I'm ok with not doing this in 2.0 - I agree more notice is a good idea. I do think we want to keep representation_cls as a property now, though. That will make a transition easier down the road. It would act just like representation does, accepting strings and such. It's just clearer that this is indicating the kind of representation, rather than the actual representation object.

[eteq]

Closing this in favor of #6591 (which is more concrete), but if we reject that this should be re-opened.