Switch "representation" to "representation_type" in coordinates by adrn · Pull Request #6873 · astropy/astropy

adrn · 2017-11-18T05:18:44Z

The context is: the frame classes and SkyCoord accept a representation= keyword and have a .representation attribute (and similar for differential), but these are the class, not the underlying representation object (that is .data). This has confused me, @eteq, and at least a few users, so we have a long term plan of deprecating and removing these attributes in favor of representation_type and differential_type.

This is something we discussed in the lead up to v2.0 and then recently at the coordination meeting.

This still needs:

Documentation updates
Updates to the tests: right now I left the tests (for representation at least) just to make sure that they still pass with the new attribute name / keyword.
At least one check that this doesn't break things in the wild (@Cadair checking this with Sunpy would be great!)
TODO comments over each line that will be deprecated in the future

This fixes #6591

astropy-bot · 2017-11-18T05:18:58Z

Hi there @adrn 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

mhvk

Looks like a good start, but a few too many replacements of _cls... Also, a general question that I think we should raise an exception if both representation and representation_type are present.

A more general question is whether at least for the properties we can already have a AstropyPendingDeprecation warning. (I must admit I'm less sure about the keyword case, at least for representation=<some-string> as that may well be in quite wide usage, so we'd at the very least have to be very slow - maybe pending now, deprecation in 4.0, removal in 5.0).

mhvk · 2017-11-18T15:33:48Z

    ----------
-    representation : `BaseRepresentation` or None
-        A representation object or `None` to have no data (or use the other
+    representation_type : `BaseRepresentation` or None


I think this one should be changed to data - since here you are allowed to put in a BaseRepresentation instance.

Although I always found it annoying that in fact the initializer is *args. But that is independent of this PR?

Yea, the *args thing annoys me too...I wonder if there is any way to use the metaclass to hack the signature? Sounds a little scary though. But yea, I think outside of the scope of this PR.

mhvk · 2017-11-18T15:34:14Z

-        Coordinates, with names that depend on the subclass.
-    differential_cls : `BaseDifferential`, dict, optional
+        Coordinate components, with names that depend on the subclass.
+    differential_type : `BaseDifferential`, dict, optional


And above you need to add representation_type (as in the initializer below).

I also note that the line is confusing: it should be

differential_type: `BaseDifferential` subclass, str, dict, optional

mhvk · 2017-11-18T15:39:48Z

+        # TODO: this is here for backwards compatibility. It should be possible
+        # to use either the kwarg representation_type, or representation.
+        # In future versions, we will raise a deprecation warning here:
+        representation_type = kwargs.pop('representation', representation_type)


This should be an error if representation_type is given as well. So,

if 'representation' in kwargs: if representation_type is not None: raise representation_type = kwargs.pop('representation')

mhvk · 2017-11-18T15:41:57Z


        # TODO: we should be able to deal with an instance, not just a
-        # class or string for representation and differential_cls.
+        # class or string for representation and differential_type.


This comment is actually misleading, as currently the class is able to deal with an instance for representation (i.e., the first argument can be a BaseRepresentation instance. So, the question would be whether we should be able to deal with a Differential instance as well. Personally, I don't think this is necessary - one can just pass in a representation with differential already attached.

Ok! Removing this comment.

mhvk · 2017-11-18T15:45:50Z

-        elif self.representation:
-            representation_cls = self.representation
+        elif self.representation_type:
+            representation_type = self.representation_type


I think here it is actually clearer to stick with representation_cls = self.get_representation_cls() (i.e., use the getter), because the whole stanza below deals with something known to be a class.

mhvk · 2017-11-18T15:53:53Z

        Can only be passed in as a keyword argument.

-    differential_cls : `BaseDifferential`, dict, optional
+    differential_type : `BaseDifferential`, dict, optional


Strange that the str option was missing again. See comment for Baseframe in that it also should be BaseDifferential subclass. I guess this holds for all other frames too. Sigh.

It may be worth using astropy.utils.decorators.format_doc...

OK, I sucked it up and overhauled all of the docstrings to use format_doc. I did it in a single commit, so it's probably easiest to look at the diff for that commit...

mhvk · 2017-11-18T15:56:48Z

        coord_kwargs = {}
        if 'representation' in kwargs:
-            coord_kwargs['representation'] = _get_repr_cls(kwargs['representation'])
+            kwargs['representation_type'] = kwargs.pop('representation')


I'd raise if representation_type is present already in kwargs

mhvk · 2017-11-18T15:57:58Z

+        # TODO: deprecate this in future
        if 'representation' in kwargs:
-            valid_kwargs['representation'] = _get_repr_cls(kwargs.pop('representation'))
+            valid_kwargs['representation_type'] = _get_repr_cls(


Again, raise if representation_type is already present.

mhvk · 2017-11-18T15:59:09Z

+    # TODO: deprecate this in future
    if 'representation' in kwargs:
-        frame = frame_cls(representation=_get_repr_cls(kwargs['representation']))
+        repr_cls = _get_repr_cls(kwargs['representation'])


And again. Also, I'd set representation_type as you have above. It does seem like you could make a mini-helper function _normalize_representation_type(kwargs)...

mhvk · 2017-11-18T16:00:31Z

    icrs = ICRS(ra=1*u.deg, dec=60*u.deg,
                pm_ra=10*u.mas/u.yr, pm_dec=-11*u.mas/u.yr,
-                differential_cls=r.UnitSphericalDifferential)
+                differential_type=r.UnitSphericalDifferential)


Hmm, don't we test passing in a string?

I added a test for this.

Cadair · 2017-12-08T15:03:32Z

I do need to check this works with SunPy. The change will mean some refactoring for us when we are astropy >= 3.0 only. (Which will not be our next release as we have one more 2.7 release scheduled)

adrn · 2017-12-08T16:01:42Z

@Cadair This PR shouldn't break sunpy (we should test this when I get farther along) as it doesn't deprecate or remove using "representation", it just adds the new name representation_type as the name that actually stores the info. Setting/getting .representation will just pass through to representation_type. This is just the first step towards eventually deprecating and removing.

adrn · 2017-12-20T02:15:33Z

OK I think I addressed @mhvk's comments. The diffs are pretty ugly - sorry about that...One major thing I did (in 653395e) is to overhaul the builtin frame docstrings to use the format_doc() decorator.

I added a test for backwards compatibility, and improved one of the frame initialization tests that tries a bunch of valid argument combinations. Next commit(s) will be documentation updates.

taldcroft · 2017-12-20T11:52:51Z

From a brief scan of diffs from the API perspective this looks good to me and matches how we discussed handling this at the coordination meeting. Thanks!

@Cadair - your SunPy testing will be crucial as the idea we agreed on is that this change should not break any code right now, even testing that is being strict about warnings. (I.e. in this release there won't even be pending deprecation warnings, just doc / example updates that reflect the new preferred API).

@mhvk - yes, the idea is to take this very slowly.

taldcroft · 2017-12-20T12:00:25Z

This change should be explicitly discussed somewhere in the narrative docs. Maybe a .. note:: at the top of the Reference/API section in the coordinates index.rst? We don't always bump things like this up to the main docs, but in this case I can imagine people just being confused that this keyword has changed without explanation.

mhvk

This looks good; only small comments left.

mhvk · 2017-12-20T13:55:17Z

+                    # This then causes the dictionary key check to fail (i.e.
+                    # comparison against `diff._get_deriv_key()`)
                    data._differentials.update({'s': diff})
                    # data = data.with_differentials({'s': diff})


Commented-out code should be removed!

mhvk · 2017-12-20T14:01:59Z

        return out


+def _normalize_representation_type(kwargs):


Move this to baseframe, use it there as well, and import it here? Would help ensure that we deprecate at the same time.

mhvk · 2017-12-20T14:03:40Z

+    # TODO: deprecate these in future
    @property
    def representation(self):
-        return self.frame.representation


Might leave these as is, again to ensure we do things at the same time.

mhvk · 2017-12-20T14:06:31Z

    repr_gal = repr(gal)

    for k in kwargs:
+        if '_type' in k:


Why? Shouldn't only differential_type be skipped?

Cadair

This appears to cause no obvious issues with SunPy.

eteq

I think I found the problem that was making the tests fail and pushed up a commit. Assuming that works I'm happy with this aside from two things:

Needs a changelog entry (both as a "new feature" and an API change
I agree with @taldcroft that it's worth just a few-sentence note in the narrative docs on what happened here.

eteq · 2017-12-22T09:17:55Z

+        # This is here for backwards compatibility. It should be possible
+        # to use either the kwarg representation_type, or representation.
+        # TODO: In future versions, we will raise a deprecation warning here:
+        _normalize_representation_type(kwargs)


this looks to me to be the source of all the failures: _normalize_representation_type is expecting representation_type to be in the kwargs, but it isn't - it's a separate stand-alone keyword. If I add this immediately above:

if 'representation' in kwargs: representation_type = kwargs.pop('representation')

most of the tests begin to pass. So from that I derived the commit I pushed up to this branch that I hope should get the tests all passing.

adrn · 2017-12-22T21:57:00Z

Why does this need a changelog entry in new features? I added a line in the API changes section. Can you add one to new features if you think it is necessary?

Your commit seems to have fixed the build failures - thanks

adrn · 2017-12-22T22:00:15Z

Also, I ran out of time: this still needs a few sentences in the documentation to explain the change. I can do this much later tonight unless someone gets to it first.

…docs

adrn · 2017-12-23T15:57:27Z

Oof, that was a pretty nasty rebase. I think I cleaned up any possible issues, but it would be good for someone to take a look at the line changes again. The main files with conflicts were baseframe.py and sky_coordinate.py.

I also added a .. note:: to the documentation explaining the change.

[ci skip]

addressed

eteq · 2017-12-23T23:21:59Z

(I guess I didn't say this above, but I looked and other than a changelog tweak it looked good to me - @adrn seems to have strong rebase-fu!)

adrn added coordinates Work in progress labels Nov 18, 2017

adrn added this to the v3.0.0 milestone Nov 18, 2017

adrn requested review from Cadair, eteq and mhvk November 18, 2017 05:18

mhvk previously requested changes Nov 18, 2017

View reviewed changes

adrn force-pushed the coordinates/rep-type branch from 0567c3c to 9adfa35 Compare December 20, 2017 02:11

mhvk reviewed Dec 20, 2017

View reviewed changes

Cadair approved these changes Dec 21, 2017

View reviewed changes

adrn changed the title ~~[WIP] Switch "representation" to "representation_type" in coordinates~~ Switch "representation" to "representation_type" in coordinates Dec 21, 2017

adrn added Ready-for-final-review and removed Work in progress labels Dec 21, 2017

eteq requested changes Dec 22, 2017

View reviewed changes

adrn mentioned this pull request Dec 22, 2017

Enable velocities in SkyCoord #6944

Merged

adrn force-pushed the coordinates/rep-type branch from 7244e19 to d2355dd Compare December 22, 2017 21:58

adrn added 6 commits December 23, 2017 10:20

find and replace differential_cls

9b5a7ff

change representation to representation_type

36d14e1

edit docstring and accept string for differential_type

a67d144

raise error if old and new names both passed

ac48780

add test of backwards compatibility

7b957a9

revert some greedy replacement of _type

67b2e18

adrn and others added 11 commits December 23, 2017 10:30

Update all builtin frame docstrings to use a template and format_doc

b0d9561

test passing classes and strings to _type args

fc450f3

clean up replacing representation with representation_type in skycoord

edefd11

change representation= and .representation to representation_type in …

ad20d3c

…docs

blank line

3b0a05a

comments by mhvk

b3d4e6f

make baseframe initialize properly normalize representation_type

486159a

remove comments

86a008d

changelog entry

c3544e4

new line in changes because of rebase

afe7f6c

clean up rebase issues

baa34f0

adrn force-pushed the coordinates/rep-type branch from d2355dd to baa34f0 Compare December 23, 2017 15:46

adrn added 2 commits December 23, 2017 10:50

resolve more rebase issues

d951570

add note to docs explaining the change

74da4c6

Changelog clarification

9aa2b9f

[ci skip]

eteq approved these changes Dec 23, 2017

View reviewed changes

eteq merged commit 8ca5d0c into astropy:master Dec 23, 2017

eteq mentioned this pull request Dec 23, 2017

Deprecate representation in frames and SkyCoord #7032

Closed

eteq mentioned this pull request Jan 20, 2018

Fix 7069 #7104

Merged

pllim mentioned this pull request Jun 21, 2021

References to older releases described as current in documentation. #11859

Closed

Conversation

adrn commented Nov 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astropy-bot Bot commented Nov 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrn Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cadair commented Dec 8, 2017

Uh oh!

adrn commented Dec 8, 2017

Uh oh!

adrn commented Dec 20, 2017

Uh oh!

taldcroft commented Dec 20, 2017

Uh oh!

taldcroft commented Dec 20, 2017

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cadair left a comment

Choose a reason for hiding this comment

Uh oh!

eteq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrn commented Dec 22, 2017

Uh oh!

adrn commented Dec 22, 2017

Uh oh!

adrn commented Dec 23, 2017

Uh oh!

eteq commented Dec 23, 2017

Uh oh!

Reviewers

Assignees

Labels

adrn commented Nov 18, 2017 •

edited

Loading

astropy-bot Bot commented Nov 18, 2017 •

edited

Loading

adrn Dec 20, 2017 •

edited

Loading