Add replicate and replicate_without_data methods to frames by eteq · Pull Request #6182 · astropy/astropy

eteq · 2017-06-12T06:04:57Z

This is a straightforward PR that adds a clone_frame method to BaseCoordinateFrame. This method copies the frame and its attributes, without the data, optionally overriding frame attributes.

I'm not a huge fan of the specific name clone_frame, as it doesn't clearly convey that the data get removed. But I felt unrealize_frame doesn't convey the fact that this is also a good way to copy attributes (overwriting specific ones if desired). So this seemed like a compromise... But I'm willing to be convinced if someone has strong opinions.

This idiom is used in a variety of places, and this method can probably be used to clean things up. But for now just getting it in for 2.0 is a good starting point. (Note that it also makes some of the velocity work @adrn and I are finishing up easier).

cc @astrofrog or @Cadair (one of you I think originally suggested this in some form or another... I think?)

adrn · 2017-06-12T11:49:43Z

Cool, sounds reasonable to me. I don't love the name though - just brainstorming:

.without_data()
.copy_without_data()
.no_data()

bsipocz · 2017-06-12T11:59:55Z

+1 to .copy_without_data()

mhvk

How about just a new copy method? It could have **kwargs like here, but by default also include representation. Then, if you do not want a representation included, you put representation=None? (I think this would "just work"!).

mhvk · 2017-06-12T16:17:14Z

astropy/coordinates/baseframe.py

+            A new object with the same frame attributes as this one, but
+            with no data and possibly new frame attributes.
+        """
+        attrs_to_set = {nm:getattr(self, nm) for nm in self.frame_attributes}


This should exclude defaults (i.e., just copy frattrs definition from realize_frame)

mhvk · 2017-06-12T16:17:46Z

astropy/coordinates/baseframe.py

+            with no data and possibly new frame attributes.
+        """
+        attrs_to_set = {nm:getattr(self, nm) for nm in self.frame_attributes}
+        for nm, val in kwargs.items():


Why not just frattrs.update(kwargs)?

oh, good point! Will do.

mhvk · 2017-06-12T16:24:24Z

p.s. Note that currently copy is provided by ShapedLikeNDArray -- which actually does more like a deepcopy, copying all the attributes and the data...

eteq · 2017-06-12T18:22:03Z

@mhvk - #6182 (comment) is exactly why I didn't go with copy - copy actually copies the data because it's based on ShapedLikeNDArray, whereas this is about copying everything except the data. My thinking was that it would be more confusing to overload copy() with different functionality than pick a new name...

eteq · 2017-06-12T18:24:48Z

although... now that I think about it, I kind of like the representation=None idea. The question is what to do about the default, though - should be like the current copy to not break backwards-compatibility/consistency with other ShapedLikeNDArray. Will experiment to see if I can make something work naturally.

mhvk · 2017-06-12T18:51:52Z

By analogy with Time, you could also create a replicate method (which needs some additions to _apply -- see the Time version). That also allows to override (though only one item, the format).

p.s. It is perhaps slightly unfortunate that our copy is usually deepcopy -- I think there was even an issue about this... -- but that is a ship that sailed long ago.

eteq · 2017-06-12T19:11:18Z

Hmm, a replicate might be a good idea, but I think it's a bit harder to see how that connects with the "drop data" element here. Having thought a bit more I think it's better to have this in its minimal form (in which case copy_without_data seems promising) as sort of the converse to realize_frame, mainly because it helps with the relatively confusing bit about frames that they are both frames and can carry data. It might be we also want replicate, though, for the with-data case.

mhvk · 2017-06-12T20:00:22Z

How about dataless_frame, disregard_data, lose_data_frame? In any case, it may be an idea for both the new one and realize_frame to use replicate internally (possibly channeled via _apply).

…ut_data

eteq · 2017-06-13T01:09:18Z

OK, I went the route @mhvk suggested here, with the name choice of replicate_without_data (seems like a good compromise between @adrn and @bsipocz favorite and the implementation). It's quite different now in that everything goes through _apply (including realize_frame). I think that's better and more consistent, now that we've gone through this, but it's worth another look to make sure the _apply changes look right (probably by @mhvk...).

mhvk

This looks very good, I'm happy it worked so well. Apart from the minor comments in-line, one question is whether one should just expose the possibility of setting the data, i.e., use representation explicitly instead of _framedata -- but obviously we can also add this later, so fine to keep as is too.

mhvk · 2017-06-13T09:24:22Z

astropy/coordinates/baseframe.py

        def apply_method(value):
            if isinstance(value, ShapedLikeNDArray):
-                return value._apply(method, *args, **kwargs)
+                if method=='replicate' and not hasattr(value, method):


Nitpick: spaces around ==.

p.s. Nice future-proofing!

Actually, it wasn't just future-proofing (although that is indeed an important side-benefit!). It turns out this is necessary just to get the tests to pass because Quantity sub-classes like Angle/Latitude/Longitude don't have replicate.

But in the end it's good anyway this way, I think!

mhvk · 2017-06-13T09:27:23Z

astropy/coordinates/baseframe.py

                else:
-                    return getattr(value, method)(*args, **kwargs)
-
+                    if method=='replicate' and not hasattr(value, method):


Same nitpick.

mhvk · 2017-06-13T11:47:58Z

astropy/coordinates/baseframe.py


+    def replicate(self, copy=False, **kwargs):
+        """
+        Generates a new frame object that is a copy of this one, but possibly


It is mostly a matter of taste, but I prefer single-line "titles" of the docstring. E.g., inspired by Times:

Return a replicate of the frame, optionally overriding some attributes.

mhvk · 2017-06-13T11:49:56Z

astropy/coordinates/baseframe.py

+
+    def replicate_without_data(self, copy=False, **kwargs):
+        """
+        Generates a new frame object that is a copy of this one, but without


Similarly, maybe

Return a replicate without data, optionally overriding some attributes.

(and then still some text like you have here, including the analogy with realize_frame)

mhvk · 2017-06-13T11:56:25Z

p.s. Maybe edit the title of this PR.

mhvk · 2017-06-13T12:54:50Z

(Approved changes, since really only small comments remain.)

eteq · 2017-06-13T15:18:46Z

Alright, I addressed @mhvk's comments, and I think this is ready to merge after the tests pass (or maybe even before, given that these are very minor changes).

mhvk · 2017-06-13T15:53:14Z

Agreed that running the full tests is not necessary, especially since appveyor and circle already passed. So, I cancelled travis and will merge. Nice addition!

Cadair · 2017-06-14T16:38:31Z

This seems to have broken SunPy master. It seems that our use of realize_frame is no longer valid: https://github.com/sunpy/sunpy/blob/master/sunpy/coordinates/frames.py#L364

https://travis-ci.org/sunpy/sunpy/jobs/242845641#L1123

Any bright ideas before I jump all the way into this?

eteq added 2 commits June 12, 2017 01:54

add clone_frame method

2aeab73

add test for clone_frame

25e505d

eteq added the coordinates label Jun 12, 2017

eteq added this to the v2.0.0 milestone Jun 12, 2017

add changelog entry

19fa02a

mhvk requested changes Jun 12, 2017

View reviewed changes

eteq added 3 commits June 12, 2017 15:18

implementation tweaks suggestested by @mhvk

8237610

change name to copy_without_data

a5577b2

change to using replicate and apply

342dc5c

eteq added 3 commits June 12, 2017 17:46

change to consistently use _apply including change to replicate_witho…

1729467

…ut_data

fix _apply to work correctly even when replicate is missing

7783378

fix for correct replicate behavior

fa4fb5a

update changelog

599c2a7

mhvk reviewed Jun 13, 2017

View reviewed changes

mhvk approved these changes Jun 13, 2017

View reviewed changes

eteq changed the title ~~Add clone_frame method to frames~~ Add replicate and replicate_without_data methods to frames Jun 13, 2017

eteq added 2 commits June 13, 2017 11:10

style tweak suggested by review

3a4616a

docstring clarification inspired by @mhvk review

4280c09

mhvk merged commit bf7805d into astropy:master Jun 13, 2017

eteq deleted the clone-frame branch June 13, 2017 16:02

Cadair mentioned this pull request Jun 15, 2017

#6182 Modified the behaviour of realize_frame so it changes the representation attribute of the frame #6208

Closed

mhvk mentioned this pull request Jun 17, 2017

Ensure realize_frame does not overwrite representation class. #6236

Merged

Uh oh!

Conversation

eteq commented Jun 12, 2017

Uh oh!

adrn commented Jun 12, 2017

Uh oh!

bsipocz commented Jun 12, 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!

mhvk commented Jun 12, 2017

Uh oh!

eteq commented Jun 12, 2017

Uh oh!

eteq commented Jun 12, 2017

Uh oh!

mhvk commented Jun 12, 2017

Uh oh!

eteq commented Jun 12, 2017

Uh oh!

mhvk commented Jun 12, 2017

Uh oh!

eteq commented Jun 13, 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!

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!

mhvk commented Jun 13, 2017

Uh oh!

mhvk commented Jun 13, 2017

Uh oh!

eteq commented Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Jun 13, 2017

Uh oh!

Cadair commented Jun 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eteq commented Jun 13, 2017 •

edited

Loading

eteq commented Jun 13, 2017 •

edited

Loading