Add _unitrep attr of SphericalRepresentation - #3757

This may require some discussion because it includes a subtle but significant change: adding the copy=False option means we're providing a way to break the guarantee of immutability for coordinate objects. @taldcroft @adrn @astrofrog

The _unitrep stuff is essentially independent of that, right @Cadair?

The background for this is that @Cadair was working with @ejeschke to try to speed up Ginga, and there's a need to quickly (in a GUI context) "re-populate" coordinate objects with new data, avoiding the object creation overhead.

Also, btw, @Cadair, this needs a changelog entry.

Yea, the copy kwarg could require some thought. I don't have a strong opinion about maintaining immutability or not, but it would be a significant change. I worry slightly that it could be tricky to protect against things that would break if we open up that door...

Was there a specific need to swap the order of UnitSphericalRepresentatation and SphericalRepresentatation? This just makes some irrelevant diffs, or else obscures real diffs.

If I moved the _unitrep stuff out of this, would people be willing to put it into a bug fix release seeing how it's not API breaking? (Just asking because it really breaks solar coords in Ginga).

On the topic of copy kwarg. It is by far the easiest way to speed up the realize frame call (which really needs to happen for Ginga) the other option is going mad on the BaseFrame__init__.

I personally think that it should be ok, people (like me) will hack around it anyway.

@taldcroft I swapped them because otherwise the unitrep was not defined when creating the class attr.

I think it would be better to leave realize_frame alone and have a new method like update_representation_data. That way it is more obvious that an in-place update is being done. We could also have a really big user-beware warning in that method that says there might be subtle issues from doing this and so it should be used with caution.

In general I think we need to open this box to support use cases like this where the current performance is not acceptable.

👍 to @taldcroft's suggestion here of a different name with a prominent .. warning:: section in its docs.

@Cadair - I also like your suggestion of splitting out the _unitrep stuff - at the very least that makes it easier to review which changes are for what.

This is now just the ._unitrep stuff.

@Cadair - are there any diffs in SphericalRepresentation or UnitSphericalRepresentation apart from the _unitrep class attribute? Difficult to tell because of the order swap of class definitions.

Also kindof a nit, but the coordinates package tends to use more verbose names, so _unit_representation might be more consistent. Since it only gets used in one place the verbosity shouldn't be a problem.

@taldcroft I have changed _unitrep to _unit_representation.

The only change in representation.py is the addition of the attribute to the SphericalRepresentation class, and the order swap.

@embray I have moved the changelog entry, I think it is ok now.

👍

Any more comments on this?

@Cadair - if I do the following there are a lot of matches in the coordinates code:

find astropy/coordinates -name '*.py' | xargs grep UnitSphericalRepresentation

So my question is whether some of these need to be generalized to use the new class attribute so that subclasses actually behave as expected. One obvious case is in the __repr__ method of baseframe.py. This does:

                if (issubclass(self.representation, SphericalRepresentation) and
                        isinstance(self.data, UnitSphericalRepresentation)):
                    data = self.represent_as(self.data.__class__,
                                             in_frame_units=True)
                else:
                    data = self.represent_as(self.representation,
                                             in_frame_units=True)

If it is the intent (or requirement) that custom _unit_representation classes be a subclass of UnitSphericalRepresentation then this is OK, otherwise not. Apart from that I haven't gone through and looked at each instance of UnitSphericalRepresentation in the code, but maybe you need to, or at least have justification for why no generalizations are needed.

This is leading into adding some tests of having a custom _unit_representation and putting that object through the paces.

@taldcroft I was working on the assumption that a class in the _unit_representation attribute would be required to be a subclass of UnitSphericalRepresentation are you ok with this? I will document it somewhere.

I will try and find some time to work more on this, but I am rather busy.

OK, if the _unit_representation needs to be a subclass then part of my question is answered. But I still suspect there are many cases where UnitSphericalRepresentation has been hardwired and assumed to be the only possible UnitSphericalRepresentation class. E.g.:

if self.data.__class__ == UnitSphericalRepresentation:
...

And in representation.py:

...
elif other_class is UnitSphericalRepresentation:
...
return UnitSphericalRepresentation(lon=self.lon, lat=self.lat)

No worries on time. This is your feature request after all!

Is this ready to merge, then?

See minor cleanup comments; they can be a [skip ci] commit to speed this up.

Two minor comments above, and one other thing best described via comment: Can you add just a bit in the representations.rst file mentioning this attribute's existence?

Thanks @Cadair !

I will get to this asap.

@mhvk @astrofrog @eteq I think this is ready!

Alright, I think we can merge this once/if the tests pass!

👍

Thanks everyone.