Add an update_representation_data method to BaseCoordinateFrame. - #3797

ping @eteq @taldcroft

@Cadair - to use this do you create a new representation object each time or modify an existing one in place (using private attributes, e.g. c.frame._data._lon)? I notice that you can get another factor of 10 in speed doing the latter (~25 usec vs 250 usec). One could conceivably have a method that digs down to this level for ultimate speed.

@Cadair - this could use a test that does:

Basic test:

• Make a SkyCoord
• Call update_representation_data to make an update, verify this works

More in-depth test:

• Make a SkyCoord
• Change the representation (e.g. to cartesian)
• Call update_representation_data to make an update
• Verify that the cartesian values are different now (since the rep_cache is cleared this should work, though in practice I'm not 100% that it will work).
• Change representation back to spherical
• Verify that the expected update is in place
• Do a transform and make sure that is correct for the updated data

@Cadair - sorry for the delay... I've finished my review. Definitely 👍 on the concept.

@eteq - currently you can update a coordinate in place via c.frame.data._<component> = value. Apart from the _rep_cache, do you know of anything else that will break or possibly become inconsistent if you do this in-place update? We went with the idea of being immutable to be conservative, so now the question is whether we can relax that. It's easier to answer this question now with a mature system and tons of tests available.

Any progress on this?

@Cadair - do you think you can try to squeeze in the "basic tests" @taldcroft mentioned a while back here?

And to answer the question I never answered from 11 months ago... As far as I'm aware _rep_cache is currently the only thing that needs clearing. But that might change. So @Cadair, if you do think you can get to this, maybe just move that part of the code to a private method _invalidate_cache? Then in the future we'll know that anything that needs re-setting can go there.

@Cadair - any thought that you might want this in time for the Astropy v1.3 feature freeze (this Friday)?

Alternatively, do you expect you'll get to this at any point, or should we consider closing it?

@eteq I think I addressed all the comments

Sorry to come late to this discussion, but having implemented the shape-changing stuff, I've become more aware of how coordinates work. Because of that, two comments, one on the implementation, the other on the name:

On the implementation, and specifically the lack of checks: There are really two ways one could screw up one's object: either its shape is inconsistent with the frame attributes, in which case the instance can no longer be transformed, etc., or the data is for a wrong type of frame and hence the coordinates now are wrong. I think we cannot easily protect against the latter type of error, but since realize_frame would do the wrong thing the same way, there is no real need either. But the former would not be so difficult to address. Most simply, one could insist that any data replacement that is done has the same shape as existing data. A second iteration of this would be to check that the shape is consistent with that of the frame attributes. So, something like:

if representation_data.shape != self.shape:
    <some code that checks shapes, like in __init__>
    (likely best done by factoring it out)
self._data = representation_data

The above means it wouldn't be quite as fast to replace data with other data with a different shape, but I think that is OK.

Now on the name: I feel introducing update_representation_data adds unnecessary clutter. At first, as an alternative, I thought of an in_place option for realize_frame, but I see this was rejected in #3757 and it does feel rather hackish. But, knowing that you almost certainly are circumenting the whole issue now by just setting self._data, I wondered if we shouldn't more simply offer a setter for data. I do feel this is substantially more pythonic, and actually OK as long as we do the very simple test that whatever is attempted to be stored is a BaseRepresentation instance and that it has OK shape (i.e., we just do only those checks that the initialiser would do as well).

p.s. I do wonder if it wouldn't be easier to just ensure we can make a copy of a frame much more rapidly, i.e., can we really not speed up realize_frame by a lot?

@mhvk How does the Representation objects store shape? Does each lat lon etc have a shape, which is also stored in the frame?

I would be ok with a setter for data, but I think it would be harder to expose the "this may eat your cat" warning to users with a setter over a dedicated method?

@Cadair - the representation object has a shape property, which is the broadcast version of any lon, lat, etc., input.

On the "this may eat your cat" -- my feeling is that if we check that something is a representation and that is has an OK shape, we have done everything the initialiser would do, so a large warning no longer is necessary.

But as said, I do wonder if we cannot speed up realize_frame by a lot so that we simply don't have to do this trickery any more... I'll try to look into that a bit more...

@mhvk - just to check, do you mean you can look into this more before the feature freeze deadline?

@mhvk - The purpose of this function is basically to be as fast as possible, primarily for use cases where it's a small (often scalar) coordinate object. In that case, it's hard to see any way realize_frame could ever approach in speed something like this. (Although I'm happy to be proven wrong! But I think it would take a much more serious re-write of both SkyCoord and the frame classes to make that possible.)

I think it may indeed make sense to do the shape-checking, though, as that's still a cheap operation. But I still think it's important to keep the "this may eat your cat" warning, because we just haven't thoroughly checked all of this. I also really don't want people to think this is a "standard" way of doing this. I don't think we want to encourage mutability, as there are other things that we aren't necessarily invalidating properly (e.g. the KD-tree from the searching functions), and the point of the immutibility was to make it so we don't have to. So I think it's important that this be treated as a tool to be used only in niche cases where you really definitely need the speed and know you aren't going to screw something up.

Put another, more succinct way: I do not think we want to make a data setter because that implicitly says that coordinates are mutable. And I'm just not yet comfortable with that.

Oh, and my comment above answered @taldcroft's question about mutability: the kd-tree used by the matching stuff. But that's not even implemented in SkyCoord itself, and represents the sort of user-style code that we'd be disallowing by breaking the immutibility currently present.

I agree with @eteq on the data setter front, I don't think it's such a good idea.
I can add a shape check but I don't really know what form that needs to take.

Let me just toss out a plan B for this that occurred to me in looking at this today.

It seems actually possible to set the representation data in place using "public" methods (apart from cache clearing). By using numpy broadcasting then there is no chance of getting the shape wrong, hence no need to additional validation. Hopefully @mhvk won't point out some subtlety in this which breaks everything. 😄

In [70]: c = SkyCoord([1,2],[3,4], unit='deg')
In [71]: c.data.lon[()] = [10, 20] * u.deg
In [72]: c.frame._rep_cache.clear()
In [73]: c
Out[73]: 
<SkyCoord (ICRS): (ra, dec) in deg
    [( 10.,  3.), ( 20.,  4.)]>

In [74]: c = SkyCoord(1,2, unit='deg')
In [75]: c.data.lon[()] = 10 * u.deg
In [76]: c.frame._rep_cache.clear()
In [77]: c
Out[77]: 
<SkyCoord (ICRS): (ra, dec) in deg
    ( 10.,  2.)>

Now here's the idea:

• Add narrative documentation of this idiom for doing the in-place update. That means people cannot accidentally discover this without being in the narrative docs and seeing the red warnings.
• Basically this is an advanced method and thus we don't make it too easy.
• Make it the burden of developers to clear the cache via the new public method.
• This method removes a lot of the potential breakage because it only allows in-place update of the values in the existing representation, NOT wholesale replacement of the representation.

I like this approach, I am also very 👍 on making the cache clear public.

I feel that maybe having this PR for a complete representation swap might still be useful, but I am not sure of a performance based use case for it as @taldcroft's suggestion fixes the intended use for this PR very well.

BTW, I would naively expect the following to work as well, but it doesn't. There is probably a good reason (that I've forgotten), but it looks like accessing the ra attribute is making a copy somewhere?

In [79]: c.ra[()] = 20 * u.deg
In [80]: c.frame._rep_cache.clear()
In [81]: c
Out[81]: 
<SkyCoord (ICRS): (ra, dec) in deg
    ( 10.,  2.)>

That must be happening at the Representation level right?

@taldcroft - for why your second example doesn't work, see #4903, i.e., my question whether it was possible to revert the decision to present ra, dec always in degrees to the users -- which (sadly) implies that skycoord.ra is not skycoord.frame.data.lon.

On the more general PR question: I like the approach of pointing out how one can update the values in-place, and what else needs to be done to reinitialize. Note that, effectively, this means you keep the shape of the skycoord, but that seems by far the most common case, so is fine.

That must be happening at the Representation level right?

The representation knows nothing about ra, that translation from lon to ra happens in the frame. Here is where this is happening, in BaseFrame.__getattr__:

            rep = self.represent_as(self.representation, in_frame_units=True)
            val = getattr(rep, self.representation_component_names[attr])
            return val

So self.represent_as() is returning a copy here because in_frame_units is True. This makes a different version of the representation that is cached with in_frame_units=True, which then gets accessed when you do c.frame.ra.

Upshot is that the reliable way is accessing the representation data object directly and manipulating the repr attributes (lon, lat) not frame attributes (ra, dec). This is one more layer of difficulty that advanced developers will be able to master.

@mhvk - re: revisiting decision. That is going to be a very hard sell since it will break code.

In [101]: c = SkyCoord('1h2m3s 4d5m6s')

In [102]: c
Out[102]: 
<SkyCoord (ICRS): (ra, dec) in deg
    ( 15.5125,  4.085)>
In [104]: c.data.lon
Out[104]: <Longitude 1.0341666666666667 hourangle>

So anybody who uses c.ra.value in their non-quantity-aware code will have their code suddenly start failing depending on the input format.

As for how to make clearing the representation cache public: in Time, we have a similar cache for different time scales and formats, and we ended up deciding to make the cache itself a public property (i.e., time.cache), so that it can be cleared simply by deleting it. See https://github.com/astropy/astropy/blob/master/astropy/time/core.py#L1075 (note that lazyproperty defines a method for deleting).

@taldcroft - yes, I know it is a hard sell... Anyway, if I'll think of another good counterargument, I'll add it to #4903.

p.s. As for the cache, like for Time, I would make an entry 'representation' in it which contains the different representations that have been used, so that, if we ever want to cache them, we could also add 'realization' later.

my only concern with having the cache in an attribute is it would make it more complex to have it clear multiple caches. While at the moment we just have the representation cache I know @eteq wanted to make the clear method a place to wipe out all data cache.

@Cadair - since it is a cache, it should always be fine to just wipe it completely. We do the same in Time (where there are format and scale components to the cache).

I changed labels, as I think the consensus would seem to be that we expose the cache and write documentation, which would be a different PR.

@mhvk I am not sure I understand, my point was if we cache more than just representation then you have to delete multiple things rather than just calling wipe_cache().

Now I'm confused: if we cache everything in a dict (with possibly entries that themselves are dict), why can we not just clear all caches by deleting that dict? This is what we do in Time...

Ok that makes sense. So we have a cache attribute that has a representation key?

Yes, that would be the idea. With that, we can, if we ever wish, add one that has realization as well.

Note the implementation using defaultdict(dict) in Time -- it means you can count on self.cache['representation'] to exist (albeit being empty) even after wiping the cache.

Ok I will open up a new PR with these suggestions, probably tonight, but maybe tomorrow in the hope of sqeezing it into 1.3

👍 to the new idea from me... @Cadair, maybe this is just what you mean, but the idea would be that the cache could then be used at the user level, too, right? So e.g. the kD-tree stuff should go in the cache, rather than as private attributes as it is right now? If so, I like that idea a lot!