Ensure realize_frame does not overwrite representation class.#6236
Ensure realize_frame does not overwrite representation class.#6236eteq merged 2 commits intoastropy:masterfrom
Conversation
|
Did you mean to reference #6208? |
|
Yes, now updated the top entry so that we close the proper one if this is merged! |
|
@Cadair - apart from review, I of course mostly hope to hear that this in fact solves the regression! |
astropy/coordinates/baseframe.py
Outdated
| from .frame_attributes import FrameAttribute | ||
|
|
||
| # Import all FrameAttributes so we don't break backwards-compatibility | ||
| # Import all orher FrameAttributes so we don't break backwards-compatibility |
| # (some users rely on them being here, although that is not | ||
| # encouraged, as this is not the public API location.) | ||
| from .frame_attributes import * | ||
| from .frame_attributes import ( |
There was a problem hiding this comment.
Why did you change this back? The argument for * was that we then don't have to remember to update this if we add any more FrameAttribute classes...
There was a problem hiding this comment.
This was partially to keep my is-something-imported checker happy, but mostly because ideally people do not import the attributes from baseframe. This way, we don't break backwards compatibility, but if we define a new attribute and someone wants to use it, it will break, and they'll realize that they should change...
astropy/coordinates/baseframe.py
Outdated
| # Here we pass representation=None to _apply, since we do not want | ||
| # to insist that the realized frame has the same representation as | ||
| # self. [Avoids breaking sunpy; see gh-6208] | ||
| # TODO: expose this? But unhandy change of name, since in the |
There was a problem hiding this comment.
In #6219 I added representation_cls and am planning to deprecate representation - I guess you could use _representation_cls just to be consistent? I don't really care.
There was a problem hiding this comment.
Ah, that would be great! It is such a pain as is. So, I changed the name here, but didn't yet expose it, as I'd like to focus on just fixing the regression.
astropy/coordinates/baseframe.py
Outdated
| else: | ||
| data = self.data if self.has_data else None | ||
|
|
||
| representation = kwargs.pop('_repr_cls', self.representation) |
Cadair
left a comment
There was a problem hiding this comment.
This fixes the issue. Thanks a lot.
| super(MyFrame, self).__init__(*args, **kwargs) | ||
| if not _rep_kwarg: | ||
| self.representation = self.default_representation | ||
| self._data = self.data.represent_as(self.representation) |
There was a problem hiding this comment.
So we don't actually call represent_as in the __init__ we just set the representation attribute and set _data equal to the rep we make.
There was a problem hiding this comment.
Happy to hear this fixes the problem! Note that this is the BaseRepresentation.represent_as which should be equivalent to the explicit initialization you do in sunpy -- I just wanted to make an example that didn't depend on it.
fixes #6208
And also an alternative to #6208 which I think is slightly cleaner.