Add BaseCoordinateFrame.cache and doc inplace modification. by Cadair · Pull Request #5575 · astropy/astropy

Cadair · 2016-12-07T22:34:11Z

This is the result of the discussion in #3797 It adds a section to the documentation about inplace modification of data and the requirements to clear the new cache when doing this.

This also refactors the cache to be a lazyproperty cache of BaseCoordinateFrame which is a defaultdict(dict) and the _rep_cache is replaced by cache['representation']

Cadair · 2016-12-07T22:37:22Z

ping @taldcroft @eteq @mhvk

taldcroft · 2016-12-07T23:21:06Z

+modifiying the values of the representation data as follows::
+
+    c = SkyCoord([1,2],[3,4], unit='deg')
+    c.data.lon[:] = [10, 20] * u.deg


Use c.data.lon[()] = .. since this syntax also works for a scalar (which is a 0-d array internally).

Might want to comment on the difference between lon and ra. Then give a hint about mapping from frame attribute names to representation component names by examining c.representation_component_names.

taldcroft · 2016-12-07T23:22:53Z

+`~astropy.coordinates.BaseCoordinateFrame.realize_frame`, this is the safest way
+to change the data in a frame as it creates a new frame with that data.
+
+Due to the fact that this creates a new frame it is quite slow, and in some


You could leave out the quite, and perhaps even use relatively.

taldcroft · 2016-12-07T23:28:33Z

+changes to the frame for maximum speed. This modification can be done by
+modifiying the values of the representation data as follows::
+
+    c = SkyCoord([1,2],[3,4], unit='deg')


You should make these tested examples and show that it actually works.

taldcroft · 2016-12-07T23:32:13Z

+clear this cache so future calculations of new representations are recomputed
+using the new data. This can be achieved by doing::
+
+      del c.frame.cache['representation']


If you write del c.cache['representation'] then the example should work for either a SkyCoord or Frame. (I think so anway, needs to be verified).

taldcroft · 2016-12-07T23:33:41Z

+clear this cache so future calculations of new representations are recomputed
+using the new data. This can be achieved by doing::
+
+      del c.frame.cache['representation']


Obvious, but this PR needs tests and changelog. In general the new code looks OK to me.

taldcroft

Minor tweaks, otherwise this looks good to merge.

taldcroft · 2016-12-08T11:14:57Z


+  - A new ``BaseCoordinateFrame.cache`` dictionary has been created to expose
+    the internal cache. This is useful when modifying representation data
+    inplace without using ``realize_frame`` [#5575]


Needs a period at the end of the sentence. Then add one more sentence "Added new documentation describing details of such inplace modification of an existing coordinate object." Though we don't typically mention doc updates in the changelog, in this case it seems worthwhile.

taldcroft · 2016-12-08T11:18:36Z

+    del i.cache['representation']
+
+    # Old repr and new repr should be different (this checks for removal of the cache)
+    assert a != repr(i)


Test that it actually worked as expected: i.ra == 10 * u.deg, i.dec == 2 * u.deg.

taldcroft · 2016-12-08T11:26:36Z


 will not work as a different representation object is used when acessing the
-aliased component names.
+aliased component names. If you wish to inspect the mapping between frame


I think that splitting a sentence with a code fragment is difficult to read, and it's usually better to say what's important about a code snippet as a complete sentence prior to the snippet. So here: "... for components on the frame. For instance the following example will not work ... component names::".

Likewise in the next bit, say " ... you can look at the following dictionary::".

taldcroft · 2016-12-08T11:30:33Z

Test failure looks unrelated (network related?), I restarted the job.

mhvk · 2016-12-08T13:02:47Z

@Cadair - this looks good! One comment (both for the docstring and the documentation): since cache can only contain things that can be recalculated, one can safely delete the whole cache with del sc.cache. My own sense is that we should suggest that version -- if the coordinates change, presumably everything else in the cache is invalidated too. (This is also what we do in Time.) Note that you do not need to change any code: the lazyproperty will remove the cache, and generate a new dict when sc.cache is accessed again.

Cadair · 2016-12-08T15:15:51Z

@mhvk from my testing it appears that doing del SkyCoord.cache does not work whereas del BaseCoordinateFrame.cache does, presumably due to the way that the attr lookup works. I could also foresee a situation where some transformation is cached based on the frame attrs (not the data) I wonder if @eteq has any insight on that.

Cadair · 2016-12-08T17:10:36Z

also ping @ejeschke / @pllim on this as it was originally crafted to enable more performant mouse over in Ginga when using the AstropyWCS2 class.

taldcroft · 2016-12-08T17:27:21Z

@Cadair - can you confirm that the current documentation example del c.cache['representation'] works as expected for both a SkyCoord and Frame? I think that it should, and your assessment of why del c.cache does not clear the cache makes some sense (though I would not bet my house on it).

pllim · 2016-12-08T17:28:01Z

I was just an end-user in that respect, so I don't have much to add except being curious how this affects ejeschke/ginga#113, ejeschke/ginga#181, and ejeschke/ginga#254. The last time I tried to use it (which was a while ago), AstropyWCS2 threw exception on header without WCS.

Cadair · 2016-12-08T17:42:19Z

@taldcroft yeah I tested it for both. And in fact the doctest does it for SkyCoord and the test does it for frame.

@pllim I will try and get back into ginga at some point and see how what I have learned from all this plays out.

mhvk · 2016-12-08T19:21:37Z

@Cadair - yes, I can see that it wouldn't work -- we'd need to define a SkyCoord.__delattr__ to go with the __getattr__ that already exists. My own sense is that it is actually important to remove the whole cache -- once coordinates are changed, it is like creating a new coordinate.

Cadair · 2016-12-08T21:54:53Z

@mhvk I am not sure I agree with that, a frame with data is explicitly a combination of a frame and some data, if you change the data the frame doesn't change. I can foresee a situation where we cache something related to the frame and not related to the data (i.e. a transformation matrix), this should not be deleted if the data changes.

This actually goes back to my original motivation for the previous PR, the concept to me has always been that you should be able to setup a frame and then quickly swap out the data because the frame is a representation of the system and the numbers are just points in that system. So to me the realize_frame implementation is wasting one of the conceptual advantages of the frame/data separation by always setting up a new frame with the new data, as the frame should be immutable but the data isn't.

mhvk · 2016-12-08T22:36:40Z

I see your point, but was thinking more of @eteq's example of a KD-tree, which would have to be invalidated if the coordinates changed.

But my general feeling is that a cache should be very temporary (I don't like having to have one, really) and certainly one should always be free to delete it. Furthermore, how we store the representations perhaps should be an implementation detail.

Cadair · 2016-12-08T22:54:15Z

I agree with the being free to delete the cache concept, I wonder maybe if we want to have a non-data related cache in the future we could call it frame_cache or something to differentiate it from a frame which is invalid when the data is changed?

I added an example of removing the whole cache.

eteq

This is looking great, but one thing is missing: the kdtree that's used in the matching code should be updated to use the cache instead of its own private variable. I can do this myself, though, if you like?

eteq · 2016-12-08T23:05:06Z


+  - A new ``BaseCoordinateFrame.cache`` dictionary has been created to expose
+    the internal cache. This is useful when modifying representation data
+    inplace without using ``realize_frame``. Additionally, documentation for


inplace -> in-place?

eteq · 2016-12-08T23:14:29Z

+`~astropy.coordinates.BaseCoordinateFrame.realize_frame`, this is the safest way
+to change the data in a frame as it creates a new frame with that data.
+
+Due to the fact that this creates a new frame it is relatively slow, and in some


Found this sentence very hard to understand. Maybe: "Creating a new frame can be relatively slow, however, particularly for scalar coordinates. Hence some situations may require that data by changed in-place in an already existing frame object. This modification ..."

eteq · 2016-12-08T23:16:40Z

+    >>> del c.frame.cache
+
+which removes the whole cache, but can only be done on frame instances and not
+`~astropy.coordinates.SkyCoord` instances.


I think this last clause is not needed - it makes it sounds like you can't do this with SkyCoord, while what you actually mean is just that you have to do c.frame (as the example shows). So I'd just go with "which removes the whole cache from the frame object." or similar.

eteq · 2016-12-08T23:20:02Z

+This changes the longitude values for the frame, however
+`~astropy.coordinates.SkyCoord` and `~astropy.coordinates.BaseCoordinateFrame`
+cache representations of the data held within the frame as they are calculated
+to speed up future operations of different representations of the same data. If


Also had trouble with this, both understanding what it says, and the fact that it specifically mentions ['representation'] but not anything else that might be cached. So I'd change it to:

This changes the longitude values of the frame. Unfortunately, doing just this introduces problems: `~astropy.coordinates.SkyCoord` and `~astropy.coordinates.BaseCoordinateFrame` cache various kinds of information to speed up some repeated operations. So we need to tell the cache that it should be cleared so that it can be re-calculated from the new data. This can be achieved by doing:: >>> del c.cache.clear() or:: >>> del c.frame.cache

We should advertise just one way of doing it, and I think it should be del c.cache. This should be enabled per the earlier comment from @mhvk to add a __delitem__ to SkyCoord that just does del self._sky_coord_frame.cache.

Agree with @eteq that I can't really see a use-case for users doing fine-tuned management of the cache. As developers we should at least be able to assume an all-or-nothing strategy in user cache management.

eteq · 2016-12-08T23:25:57Z

Oops, did my review before reading all of this... But I think the whole point of this cache is that anyone can use it, even user code (which we can sort of think of the matching stuff with the KD trees as being an example of, as it's not really a part of the frame classes themselves). I also agree one should always be free to clear the cache, but as my comment above shows, I think there's an easy way around the del sc.cache dillema: sc.cache.clear() does the same thing as deleting the cache, as it completely clears out the contents.

And although it may not be clear from my inline comment , I definitely agree with @taldcroft that 'representation' should not be special-cased. In-place operations should either clear the whole cache or none of it - anything else is very dangerous/confusing.

Also, I don't undestand this at all:

I agree with the being free to delete the cache concept, I wonder maybe if we want to have a non-data related cache in the future we could call it frame_cache or something to differentiate it from a frame which is invalid when the data is changed?

I don't think anything in the cache would ever care about the frame, right? Isn't it always only about the data?

Cadair · 2016-12-09T15:15:36Z

I have made most of the changes but not added a __delitem__ to SkyCoord do we want this to replicate the __getitem__ where it just passes through to frame if it doesn't have the attribute?

I am not convinced it's needed, I actually like the .clear() approach and because we are dealing with advanced users here I am not sure it matters we are showing them two ways to zero a dict.

Cadair · 2016-12-09T15:17:33Z

@eteq

I don't think anything in the cache would ever care about the frame, right? Isn't it always only about the data?

You would know much better than me, I was just hypothesising about some transformation getting setup based on frame attributes and cached for later.

The general opinion seems pretty clear and I have no concrete use case anyway, so I have flipped the docs to delete the whole dict.

mhvk · 2016-12-09T16:22:31Z

+
+    assert len(i.cache['representation']) == 2
+
+    del i.cache['representation']


Just make this del i.cache for consistency?

Given what I just stated for the documentation, I think this should be i.cache.clear() instead.

mhvk · 2016-12-09T16:23:22Z



-def _get_cartesian_kdtree(coord, attrname_or_kdt='_kdtree', forceunit=None):
+def _get_cartesian_kdtree(coord, attrname_or_kdt='kdtree', forceunit=None):


Very much a personal preference, but I'd call this argument just kdt.

On reflection I agree with you @mhvk... but I'm the one to blame for this, not @Cadair ;)

mhvk · 2016-12-09T16:23:46Z

+    i.data.lon[()] = 10*u.deg
+
+    # Clear the cache
+    del i.cache['representation']


again, just del i.cache

mhvk · 2016-12-09T16:25:01Z

+    assert len(i.cache['representation']) == 0
+
+
+def test_inplace_change():


I feel we should have a test for a SkyCoord as well.

It is tested for SkyCoord in a doctest, but I can add one here as well.

mhvk · 2016-12-09T16:27:19Z

+
+    >>> c.cache.clear()
+
+or::


I'm very happy with the c.cache.clear() way of clearing the cache. I think it should be immediately obvious that this works for frames as well, so would suggest removing the alternative (one right way to do it!).

mhvk · 2016-12-09T16:29:51Z

Clearing with c.cache.clear() is definitely the way to go! (and here I am not knowing that dicts have that method...). So, my minor remaining comments are mostly about doing that in the tests too, and not even mentioning that one can also delete the cache. I also think we should have at least one test using a SkyCoord -- I would do both a scalar and an array case.

mhvk · 2016-12-09T16:31:02Z

p.s. I really like how this came together, from what was a bit of a kludge to something that made enough sense to help simplify the kdtree implementation!

also cache.clear() everywhere

Cadair · 2016-12-09T16:45:40Z

p.s. I really like how this came together, from what was a bit of a kludge to something that made enough sense to help simplify the kdtree implementation!

Yes this did turn out quite nice, and faster than the original hack as well :)

mhvk · 2016-12-09T17:02:02Z

    assert i.dec == 2 * u.deg
+
+
+def test_cache_clear_sc():


Sorry to be a nit-pick, but this should go to test_skycoord.py...

mhvk · 2016-12-09T17:35:31Z

All looks good to me now!

ejeschke · 2016-12-09T19:15:17Z

Very glad to see this come together and looking forward to testing it!

eteq · 2016-12-09T21:51:04Z

A few minor things, but it'll be easier for me to just do those and send them up to this branch... then will merge and backport to 1.3 (already confirmed with @astrofrog that backporting this is ok)

eteq · 2016-12-09T22:43:38Z

Everything passed, so merging!

Cadair · 2016-12-09T22:55:28Z

Woop! Thanks everyone for a really constructive review!

Add BaseCoordinateFrame.cache and doc inplace modification.

eteq · 2016-12-09T23:03:46Z

backported to 1.3.x in 950eb26

Cadair force-pushed the coordinate_cache branch from d730d57 to 47a269f Compare December 7, 2016 22:38

Add BaseCoordinateFrame.cache and doc inplace modification.

47a269f

taldcroft requested changes Dec 7, 2016

View reviewed changes

taldcroft added Affects-release coordinates Enhancement labels Dec 7, 2016

taldcroft added this to the v1.3.0 milestone Dec 7, 2016

Cadair added 2 commits December 8, 2016 10:46

Tweaks and tests

bd1b893

Add changelog

89bf349

taldcroft requested changes Dec 8, 2016

View reviewed changes

taldcroft self-assigned this Dec 8, 2016

Cadair force-pushed the coordinate_cache branch from 29076f5 to 4d2cd27 Compare December 8, 2016 15:41

Further improvements to inplace coord docs and tests

4d2cd27

taldcroft approved these changes Dec 8, 2016

View reviewed changes

Add an example deleting whole cache

e565dce

eteq requested changes Dec 8, 2016

View reviewed changes

Make kdtree use the new cache dict

3f7876d

other comments on inplace docs

6aa66d8

Cadair force-pushed the coordinate_cache branch from d54d699 to 6aa66d8 Compare December 9, 2016 15:11

mhvk reviewed Dec 9, 2016

View reviewed changes

Add 2d array tests and skycoord test

d091fcc

also cache.clear() everywhere

mhvk reviewed Dec 9, 2016

View reviewed changes

Move sc test to test_sky_coord

5d59bc3

eteq added 2 commits December 9, 2016 16:20

add some stronger tests for kdtree storage

cebe804

fixes based on stronger tests

51a49bd

eteq approved these changes Dec 9, 2016

View reviewed changes

eteq added 2 commits December 9, 2016 16:52

add check to ensure stored KD-tree is actually used

af16f66

minor doc tweak

12e933d

eteq merged commit 989b33c into astropy:master Dec 9, 2016

eteq added the whatsnew-needed label Dec 9, 2016

Cadair deleted the coordinate_cache branch December 9, 2016 22:53

eteq added a commit that referenced this pull request Dec 9, 2016

Merge pull request #5575 from Cadair/coordinate_cache

950eb26

Add BaseCoordinateFrame.cache and doc inplace modification.

taldcroft mentioned this pull request Jan 27, 2018

Iterating over a SkyCoord object is very slow #3323

Closed


		assert len(i.cache['representation']) == 2

		del i.cache['representation']



		def _get_cartesian_kdtree(coord, attrname_or_kdt='_kdtree', forceunit=None):
		def _get_cartesian_kdtree(coord, attrname_or_kdt='kdtree', forceunit=None):

		assert len(i.cache['representation']) == 0


		def test_inplace_change():

Conversation

Cadair commented Dec 7, 2016

Uh oh!

Cadair commented Dec 7, 2016

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!

taldcroft Dec 7, 2016 • 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!

taldcroft 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!

taldcroft commented Dec 8, 2016

Uh oh!

mhvk commented Dec 8, 2016

Uh oh!

Cadair commented Dec 8, 2016

Uh oh!

Cadair commented Dec 8, 2016

Uh oh!

taldcroft commented Dec 8, 2016

Uh oh!

pllim commented Dec 8, 2016

Uh oh!

Cadair commented Dec 8, 2016

Uh oh!

mhvk commented Dec 8, 2016

Uh oh!

Cadair commented Dec 8, 2016

Uh oh!

mhvk commented Dec 8, 2016

Uh oh!

Cadair commented Dec 8, 2016

Uh oh!

eteq 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!

eteq commented Dec 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Cadair commented Dec 9, 2016

Uh oh!

Cadair commented Dec 9, 2016

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!

eteq Dec 9, 2016 • 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!

taldcroft Dec 7, 2016 •

edited

Loading

eteq commented Dec 8, 2016 •

edited

Loading

eteq Dec 9, 2016 •

edited

Loading