Repository navigation
Remove OrderedDict from astropy.coordinates, small BaseCoordinateFrame.__init__ refactor - #10990
Conversation
|
RTD failure seems unrelated. |
|
Yeah, seems to happen quite often lately. 🤔 |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
@astrojuanlu - this looks nice! I like the gathering of those unwieldy parts of __init__ in their own methods. A few small comments about going perhaps just a little further.
p.s. Personally, I'd have slightly preferred to have the change from OrderedDict to dict in a separate PR, as it now mixes in some way trivial stuff with somewhat more tricky changes - though really what is most tricky is how the difference happens to be type-set.
What I usually do in these cases is to review only a range of commits:
But, if there are other reasons to split it, I'll happily do so :) Addressing your comments now. |
eec8bb9 to
32297ee
Compare
|
Thanks for your prompt review @mhvk! Just addressed your comments. |
32297ee to
39ffe21
Compare
|
@astrojuanlu - thanks, looks pretty good now. Some more comments on the representation inferrence - I think we are thinking along the same lines there! p.s. As you note, the ordereddict stuff is all in a separate commit, so let's leave it here. |
39ffe21 to
c929044
Compare
|
This looks quite beautiful (modulo the suggestion for a shorter name for the method). @eteq, @adrn, @StuartLittlefair - it might be good if any of you could have a quick look too; does any of you want to? (I'm ready to sign off on it otherwise.) Note that the diff looks annoyingly worse than it is - mostly this is just a refactoring, but one that makes the initialization easier to follow. |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
OK, it all looks good from my point of view. Will leave it open a few days just in case.
|
I will leave this to your capable hands @mhvk. Delving into the machinery of the coordinates classes makes my head hurt. |
|
Planning to take a look at this but very busy week -- will get to it over the weekend! |
adrn
left a comment
adrn
left a comment
There was a problem hiding this comment.
No comments from me! I think this looks great, and I like the restructuring / factorization of the init! Thanks @astrojuanlu for this!
5 participants
Description
OrderedDictfromastropy.coordinates, see Remove OrderedDict for astropy 4.3 or later #10888BaseCoordinateFrame.__init__to clarify some comments and move part of the code to two separate internal methods. It was not justified in itself, but used this opportunity to include it. The removal ofOrderedDictis in a separate commit that applies cleanly without the rest.