Skip to content

Conversation

@mhvk
Copy link
Contributor

@mhvk mhvk commented Mar 24, 2015

Following #3217, some common code between the routines ut1_utc and pm_xy should be factored out, and the former should also return a Quantity in units of seconds (which should be properly dealt with in Time).

After that, the interpolation should be improved: #1803.

@eteq
Copy link
Member

eteq commented Dec 17, 2014

👍

@mhvk
Copy link
Contributor Author

mhvk commented Mar 24, 2015

@eteq - I got around to this refactoring of iers. Could you have a look? One thing that would be nice is some tests of the PM_x, PM_y stuff in iers/test_iers -- though I guess it does get exercised through coordinates.

@taldcroft - since this touches Time, might you also be able to have a quick look? Note how iers now is a QTable -- lovely with those Quantity columns (though it also is clear that it would be even nicer if we had masked quantities; but remains annoyingly tricky without numpy changes...).

@mhvk mhvk force-pushed the iers-refactoring branch from f5d3733 to 2fe5b1e Compare April 1, 2015 15:43
@mhvk mhvk added this to the v1.1.0 milestone Apr 1, 2015
@mhvk mhvk assigned eteq Apr 1, 2015
@mhvk
Copy link
Contributor Author

mhvk commented Apr 1, 2015

Inspired by the coverage failure, I added quite a few more tests, including at least some basic ones on the polar motion. @eteq - not knowing anyone more suitable, I assigned this to you... Could you have a look? I think it is really quite straightforward, but good to have a second set of eyes.

@mhvk mhvk added the Affects-dev PRs and issues that do not impact an existing Astropy release label Apr 1, 2015
@mhvk
Copy link
Contributor Author

mhvk commented Apr 1, 2015

Note: the appveyor failure is just a time-out.

@taldcroft
Copy link
Member

@mhvk - my only question is whether the change from Table to QTable represents a user API change. From my understanding the IERS table is internal, but just checking.

Otherwise I am hoping to look at some other PRs with my name on them and leave this to @eteq.

@mhvk
Copy link
Contributor Author

mhvk commented Apr 1, 2015

@taldcroft - this is mostly for internal use, but you're right that it still is a bit of an API change, in particular in IERS.ut1_utc now returning a Quantity. I've added an entry to CHANGES.rst and will change this to affects-release.

@mhvk mhvk added Affects-release and removed Affects-dev PRs and issues that do not impact an existing Astropy release labels Apr 1, 2015
@mhvk mhvk force-pushed the iers-refactoring branch from b24b5ca to 1b56ed1 Compare April 10, 2015 17:35
@mhvk
Copy link
Contributor Author

mhvk commented Apr 10, 2015

@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...

@mhvk
Copy link
Contributor Author

mhvk commented May 22, 2015

@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 Time.

@mhvk
Copy link
Contributor Author

mhvk commented Jun 11, 2015

@eteq - would you be able to have a look soon? If not, could you ask someone else?

Copy link
Member

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.

@mhvk mhvk force-pushed the iers-refactoring branch from 1b56ed1 to 2a79a9f Compare June 11, 2015 18:39
@mhvk
Copy link
Contributor Author

mhvk commented Jun 11, 2015

@taldcroft - thanks, corrected (and rebased, just to be sure).

@taldcroft
Copy link
Member

@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.

@mhvk
Copy link
Contributor Author

mhvk commented Jun 12, 2015

@taldcroft - I looked again at the CHANGES.rst and I think the API changes section is a bit more logical, especially as this really is a minimal change, just making the output consistent between time offset and polar motion. I'll go ahead and merge so I can continue on Time shape changes, etc.

mhvk added a commit that referenced this pull request Jun 12, 2015
Small clean-ups of IERS table reader
@mhvk mhvk merged commit 6d0e5b0 into astropy:master Jun 12, 2015
@mhvk mhvk deleted the iers-refactoring branch June 12, 2015 01:09
@taldcroft
Copy link
Member

@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.

@mhvk
Copy link
Contributor Author

mhvk commented Jun 12, 2015

OK, no problem.

@eteq
Copy link
Member

eteq commented Jun 13, 2015

@mhvk @taldcroft - sorry I never got to reviewing this - but the good news is that I "post-reviewed" it and saw no problems!

@eteq
Copy link
Member

eteq commented Jun 13, 2015

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).

@mhvk
Copy link
Contributor Author

mhvk commented Jun 13, 2015

@eteq - thanks for the post-review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants