Let IERS table used in time/coordinates determined by science state by mhvk · Pull Request #9244 · astropy/astropy

mhvk · 2019-09-16T21:18:32Z

Addresses the first item in #9227, that it would be good to allow users to determine what IERS table is used for all time and coordinate transformations in astropy (specifically, this is to help PINT use an up-to-date IERS-B file rather than IERS_Auto).

Here, this is done with a new iers.earth_rotation_table science state class, which is check in time and coordinates.

cc @aarchiba

EDIT: changed name to earth_rotation_table

aarchiba · 2019-09-17T23:08:55Z

Very nice! But what about the global variables iers.IERS.iers_table and friends? Those will by default evade the control of earth_rotation_table. They don't seem to be much used in astropy itself (apart from IERS_Auto's no-network fallback), but are we setting up users for an unpleasant surprise when they use them?

As it stands, they always have the value of "the last IERS table of this class that was opened". They are documented in the docstrings in iers.py. They make it awkward to obtain an IERS_Auto instance that isn't shared with anyone (you have to manually reset IERS_Auto.iers_table to None before and after your IERS_Auto.open()) - and if you don't, since IERS_Auto is mutable it can mutate under you if someone else requests a new value while you're working.

mhvk · 2019-09-18T00:37:57Z

@aarchiba - very good points. Might it make most sense to deprecate cls.iers_table? When I wrote the initial IERS_A and IERS_B classes, the ScienceState class did not yet exist, and IERS_Auto has been pasted on a bit.

It would seem reasonably unlikely that anybody except PINT wants full control over the IERS tables, so I mostly want to be sure that we get it right for PINT!

aarchiba · 2019-09-18T00:45:48Z

@aarchiba - very good points. Might it make most sense to deprecate cls.iers_table? When I wrote the initial IERS_A and IERS_B classes, the ScienceState class did not yet exist, and IERS_Auto has been pasted on a bit.

I do think those should disappear. Shame we can't (easily) make them properties that emit a DeprecationWarning when accessed... I can do the metaclass thing if you want, it does work. But maybe we just need to think through what users (if any) actually expect from these class attributes and only support that.

It would seem reasonably unlikely that anybody except PINT wants full control over the IERS tables, so I mostly want to be sure that we get it right for PINT!

I was thinking about this - if we could count on IERS_Auto to keep IERS B up to date, and if we could just look at the columns in it that carry untouched IERS B data, we could drop the keep-things-up-to-date code we now have. Of course, as long as we're supporting python 2 we also need to work with astropy 2.0, so this is a down-the-road consideration. But I'm not sure astropy actually needs separate auto-updating IERS A and B classes.

bsipocz · 2019-09-18T01:09:29Z

Of course, as long as we're supporting python 2 we also need to work with astropy 2.0, so this is a down-the-road consideration.

Not sure whether "we" is PINT or astropy here. If the latter, than we're approaching to the final drop of python2 super quickly, so no need to include that in any considerations.
And even if it's PINT, none of these fixes are milestoned to be backported to the astropy 2.0 branch, so that case will need to have a separate workaround downstream, I'm afraid.

mhvk · 2019-09-18T01:17:10Z

@aarchiba - I think that updating IERS_B as part of IERS_Auto is definitely an option (or have an IERS_B_Auto. But better discussed as part of #9227.

As for deprecating cls.iers_table - nothing in astropy now relies on it, and it should not be too hard to implement (some combination of deprecated_attribute and classproperty should do the trick). But before doing that as part of this PR, I'd like some input from @taldcroft and @eteq.

aarchiba · 2019-09-18T01:25:26Z

Of course, as long as we're supporting python 2 we also need to work with astropy 2.0, so this is a down-the-road consideration.

not sure whether "we" is PINT or astropy here. If the latter, than we're approaching to the final drop of python2 super quickly, so no need to include that in any considerations.

Sorry, yes, that was unclear. As I understand it, astropy's support of python2 is basically over; nothing since astropy 2.0 works on python2, and I don't expect this to make it in to that maintenance branch. So as long as PINT supports python2, it needs to work with that aging version of astropy.

PINT's support of python2 is under discussion, but a number of key pieces of pulsar software do not work on python3, including one by one of the PINT maintainers. (And of course they are awkward to compile and install, so users may delay upgrading them.) So there will be a desire for PINT to keep working with python2 for some time yet.

mhvk · 2019-10-10T21:15:27Z

@eteq, @taldcroft - would you be able to review this new ScienceState for IERS? I think it is in good shape (code coverage failure is a red herring), but an outstanding question of whether to deprecate the caching done on the IERS classes proper. My own sense is that we can deprecate, but it is also totally fine to punt that to another issue/PR, and at least get this PR in.

taldcroft

Looks generally good. I didn't know about ScienceState! I reviewed the parts relevant to time and left some minor comments.

taldcroft · 2019-10-22T21:29:40Z

        return table
+
+
+class earth_rotation_table(ScienceState):


Might it be worth overriding the __repr__ so it does not try to print the whole IERS table? That isn't really helping out, and basically <ScienceState earth_rotation_table: IERS_A> is what users would want to see.

Good idea. Since for that it is by far the easiest to adjust ScienceState as well, let me also ping @astrofrog (@astrofrog, I changed it such that the context repr never is more than 1 line)

@astrofrog - see a0b46b0

taldcroft · 2019-10-22T21:30:52Z

+    --------
+    To temporarily use the IERS-B file packaged with astropy:
+
+    >>> from astropy.utils import iers


Don't these code blocks need to be indented with a leading ... packaged with astropy:: (two colons)? Or maybe I'm missing advances in sphinx/rst docs?

Yes, I think those were not run the way I had it. Corrected...

taldcroft · 2019-10-22T21:51:11Z

            assert allclose_sec(t._delta_ut1_utc, -0.58682110003124965)


-@pytest.mark.skipif('not HAS_IERS_A')


This diff is unfortunately not helpful, but I did a by-eye compare of the files for master and this branch. I'm just trying to make sure the existing test has not been changed substantively. You did change the way the delta and status values are computed. I think it's equivalent, but maybe life would be easier if you just left in the original two tests test_ut1_iers_A and test_ut1_iers_Auto and then add tests on top? This is finicky code so any time I see an existing test modified it rings bells.

I note that in the new do_ut1_prediction_test there is no check on the jd value being different for the non-context manager case.

def test_ut1_iers_A(self): tnow = Time.now() iers_a = iers.IERS_A.open() tnow.delta_ut1_utc, status = iers_a.ut1_utc(tnow, return_status=True) assert status == iers.FROM_IERS_A_PREDICTION tnow_ut1_jd = tnow.ut1.jd assert tnow_ut1_jd != tnow.jd

OK, I reorganized the file so the diff is more obvious, and re-instated the .jd test (I had removed it since it is possible, at least in principle, that delta_ut1_utc equals 0. Though probably sufficiently unlikely that one might ignore that - much more likely something is wrong when the jd doesn't change...

eteq

I like this pretty much as-is with just one change to suggest (and am approving on the theory that it is straightforward to implement): instead of earth_rotation_table it should be earth_orientation_table because it's not just rotation, these also have the polar motion and so on.

mhvk · 2019-10-25T18:06:42Z

My logic was that IERS stands for "Earth Rotation (and Reference System)" and "orientation" misses the leap second aspect (while rotation does include wobbling...). But if you feel strongly, I will change; need to rebase anyway...

eteq · 2019-10-25T18:10:34Z

@mhvk - IERS the organization has rotation in its name, but (admittedly, rather inconsistently), the files from their authoritative source (https://datacenter.iers.org/eop.php) are called "Earth orientation parameters", so I think "orientation". I guess really do be really pedantic it could be earth_orientation_and_rotation_table, but I think "orientation" at least could be viewed as including rotation (while vice versa does not).

eteq · 2019-10-25T18:10:44Z

(So I guess that means I do feel strongly...)

mhvk · 2019-10-25T18:25:15Z

Last try: why would rotation not include the wobbling of the axis? (Though if that doesn't convince you, I'll change it; I do not mind that much even if I could do without the work right now...)

eteq · 2019-10-25T20:08:24Z

@mhvk - to me "rotation" is only the Earth spinning on its axis, and any motion of the axis is "orientation" not "rotation". But I admit this is deep down the pedantic road...

(As a citation, I give https://en.wikipedia.org/wiki/Earth_orientation_parameters, but when I look at that it's still a bit ambiguous because it does say oreintation parameters are "irregularities in the rotation of the Earth"...)

mhvk · 2019-10-25T20:49:20Z

I'll go for earth_orientation_table...

astrofrog · 2019-10-25T20:54:01Z

Yeah orientation sounds reasonable to me (I can see arguments both ways but that'd be my choice if I had to pick a side)

Mostly following ScienceState repr change.

mhvk · 2019-10-25T22:56:27Z

OK, changed to earth_orientation_table and rebased.

bsipocz · 2019-10-25T23:09:37Z

So is this a merge if CI passes then?

mhvk added time coordinates utils Enhancement labels Sep 16, 2019

mhvk added this to the v4.0 milestone Sep 16, 2019

mhvk requested review from eteq and taldcroft September 16, 2019 21:18

mhvk mentioned this pull request Sep 16, 2019

Possible improvements to how IERS is handled #9227

Closed

1 of 7 tasks

mhvk force-pushed the iers-science-state branch 2 times, most recently from 610d35a to 70c0cd2 Compare September 17, 2019 13:55

mhvk mentioned this pull request Oct 5, 2019

Add a classmethod to retrieve a Galactocentric frame with modern solar position/motion parameters #9322

Closed

taldcroft reviewed Oct 22, 2019

View reviewed changes

mhvk force-pushed the iers-science-state branch from 70c0cd2 to d35a45d Compare October 23, 2019 15:07

mhvk added whatsnew-needed Ready-for-final-review labels Oct 24, 2019

mhvk mentioned this pull request Oct 24, 2019

Fix loading of IERS data files #9204

Closed

eteq approved these changes Oct 25, 2019

View reviewed changes

mhvk added 9 commits October 25, 2019 18:42

Add earth_rotation_table ScienceState to IERS.

fe79489

Use earth_rotation_table science state in time.

eb26cd5

Use earth rotation table science state in coordinates.

de807c5

Add documentation about earth_rotation_table

fb96cd4

Changelog entry.

dfce05f

Ensure ScienceState context repr has only single line

0299e06

Docstring/docs changes.

eb1576c

Mostly following ScienceState repr change.

Change name to earth_orientation_table

ce0cdb0

Change-log entry rebase correction.

a0d2f27

mhvk force-pushed the iers-science-state branch from d35a45d to a0d2f27 Compare October 25, 2019 22:56

mhvk merged commit cfb1c2f into astropy:master Oct 26, 2019

mhvk deleted the iers-science-state branch November 19, 2019 22:05

		assert allclose_sec(t._delta_ut1_utc, -0.58682110003124965)


		@pytest.mark.skipif('not HAS_IERS_A')

Conversation

mhvk commented Sep 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aarchiba commented Sep 17, 2019

Uh oh!

mhvk commented Sep 18, 2019

Uh oh!

aarchiba commented Sep 18, 2019

Uh oh!

bsipocz commented Sep 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Sep 18, 2019

Uh oh!

aarchiba commented Sep 18, 2019

Uh oh!

mhvk commented Oct 10, 2019

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

taldcroft Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

mhvk Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

mhvk Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

taldcroft Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

mhvk Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

taldcroft Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

mhvk Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

eteq left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk commented Oct 25, 2019

Uh oh!

eteq commented Oct 25, 2019

Uh oh!

eteq commented Oct 25, 2019

Uh oh!

mhvk commented Oct 25, 2019

Uh oh!

eteq commented Oct 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Oct 25, 2019

Uh oh!

astrofrog commented Oct 25, 2019

Uh oh!

mhvk commented Oct 25, 2019

Uh oh!

bsipocz commented Oct 25, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mhvk commented Sep 16, 2019 •

edited

Loading

bsipocz commented Sep 18, 2019 •

edited

Loading

eteq commented Oct 25, 2019 •

edited

Loading