Repository navigation
Ensure SkyOffsetFrame can again be transformed back to base frame - #11255
Conversation
mhvk
added
Bug
coordinates
Affects-dev
| members['_default_representation'] = framecls._default_representation | ||
| members['_default_differential'] = framecls._default_differential | ||
|
|
||
| newname = name[:-5] if name.endswith('Frame') else name |
There was a problem hiding this comment.
This was, I think, trying too hard to even account for generic subclasses of SkyOffsetFrame. It is not actually useful since below a subclass of SkyOffsetFrame is created...
There was a problem hiding this comment.
Okay, I didn't know about this, and seeing it now kind of answers my previous question about what the problem was. But if you remove this metaclass are the previous changes needed? Or do you think the previous changes are still good to have in case another case like this metaclass arises?
There was a problem hiding this comment.
Yes, the changes in baseframe are still needed - I did this before the other ones. It seems a tricky business merging the attributes, keeping order, and possibly overriding ones...
| (SkyOffsetFrame, framecls), | ||
| {'__doc__': SkyOffsetFrame.__doc__}) | ||
| # Create a new SkyOffsetFrame subclass for this frame class. | ||
| name = 'SkyOffset' + framecls.__name__ |
There was a problem hiding this comment.
It is possible to generalize this, but then we need to pass on cls from __new__ below as well.
|
Hmm, something went wrong with repr, with suddenly |
| frame_attrs = frame_attrs.copy() | ||
|
|
||
| for basecls in (cls,) + cls.__bases__: | ||
| frame_attrs = {} |
There was a problem hiding this comment.
@embray - please check you agree with this change in logic. I got failures because any new SkyOffsetFrame class overrides SkyOffsetFrame.origin and then it either was not properly inserted in frame_attributes or, if I reset that one (first commit), it changed the order. I think not relying on a previous calculation of frame_attributes is probably better practice anyway.
There was a problem hiding this comment.
I admit I don't really understand the problem here since I don't know what a SkyOffsetFrame is or why it was broken. What you did here makes sense and is actually close to what I think was my original approach to the problem. I just didn't see that there was a difference and was trying to avoid two loops. It didn't occur to me that there could be base classes with metaclasses that would do something different with frame_attributes.
IIUC that's what the problem was here, in which case your solution makes sense.
|
OK, all tests passed now. |
|
@pllim - numpy dev is OK again! |
embray
left a comment
embray
left a comment
There was a problem hiding this comment.
My existing comments are more rhetorical and for edification than anything else. LGTM.
| frame_attrs = frame_attrs.copy() | ||
|
|
||
| for basecls in (cls,) + cls.__bases__: | ||
| frame_attrs = {} |
There was a problem hiding this comment.
I admit I don't really understand the problem here since I don't know what a SkyOffsetFrame is or why it was broken. What you did here makes sense and is actually close to what I think was my original approach to the problem. I just didn't see that there was a difference and was trying to avoid two loops. It didn't occur to me that there could be base classes with metaclasses that would do something different with frame_attributes.
IIUC that's what the problem was here, in which case your solution makes sense.
| members['_default_representation'] = framecls._default_representation | ||
| members['_default_differential'] = framecls._default_differential | ||
|
|
||
| newname = name[:-5] if name.endswith('Frame') else name |
There was a problem hiding this comment.
Okay, I didn't know about this, and seeing it now kind of answers my previous question about what the problem was. But if you remove this metaclass are the previous changes needed? Or do you think the previous changes are still good to have in case another case like this metaclass arises?
| '_default_representation': framecls._default_representation, | ||
| '_default_differential': framecls._default_differential, | ||
| '__doc__': SkyOffsetFrame.__doc__, | ||
| }) |
There was a problem hiding this comment.
I had actually hoped to somehow use __init_subclass__ for this, but it seems at that point it is too late to define a descriptor on the class (it doesn't seem to get bound). Anyway, this is fairly clean.
|
Thanks! A few answers just for interest's sake. |
This Meta-class creation was missed in #11099 because there was no test that showed it was a problem. Now got rid of the whole meta-class and added an appropriate test.
Fixes #11254