-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Small clean-ups of IERS table reader #3223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
👍 |
|
@eteq - I got around to this refactoring of @taldcroft - since this touches |
|
Inspired by the |
|
Note: the appveyor failure is just a time-out. |
|
@taldcroft - this is mostly for internal use, but you're right that it still is a bit of an API change, in particular in |
|
@eteq - I rebased this IERS refactor to ensure it still passed all tests. Fortunately, it does. Will you have time to look at it? I'm quite sure it is OK, since really it is just reorganisation... |
|
@eteq - would you have a chance to review? I think this is quite ready to go in, and I'd rather do it ahead of some other changes being made to |
|
@eteq - would you be able to have a look soon? If not, could you ask someone else? |
astropy/utils/iers/iers.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be astropy.table.QTable now.
|
@taldcroft - thanks, corrected (and rebased, just to be sure). |
|
@mhvk - I took an inexpert look at the code and didn't see anything obviously wrong. I looked slightly more closely at the new / updated tests and it all seems reasonable. 👍 from my perspective. I personally think that all API changes should go into the What's New document (under backward-incompatible changes), so they get proper visibility. In this case there probably aren't a huge number of IERS table users. |
|
@taldcroft - I looked again at the |
Small clean-ups of IERS table reader
|
@mhvk - sorry, didn't mean to imply taking it out of CHANGES.rst, but as a reminder to also put things like this in a more visible place for the release. But that is really a different discussion. |
|
OK, no problem. |
|
@mhvk @taldcroft - sorry I never got to reviewing this - but the good news is that I "post-reviewed" it and saw no problems! |
|
Also, @bmorris3, you might want to be aware of this in case you've started some IERS work on a branch that was before this was merged (2 days ago). |
|
@eteq - thanks for the post-review! |
Following #3217, some common code between the routines
ut1_utcandpm_xyshould be factored out, and the former should also return aQuantityin units of seconds (which should be properly dealt with inTime).After that, the interpolation should be improved: #1803.