Update ERFA leap seconds using new IERS LeapSecond class. by mhvk · Pull Request #9365 · astropy/astropy

mhvk · 2019-10-14T21:57:47Z

A separate follow-up on #9329, which uses a new astropy.utils.iers.LeapSecond class to update the ERFA leap seconds. Here, updates are either gotten from IERS or from IETF.

cc also @aarchiba, whose comments made clear that my previous attempt, which used the IERS tables, was just not going to work (see #9334).

mhvk · 2019-10-14T22:39:42Z

Hmm, clearly cannot assume that CI machines have up-to-date leap-second files in their virtual machines...

mhvk · 2019-10-15T17:45:02Z

@bsipocz - is there any way to restart circle-ci jobs? (They failed due to connectivity problems.) Or should I just close/reopen?

bsipocz · 2019-10-15T17:55:50Z

is there any way to restart circle-ci jobs?

yes, log into circleCI and there should be a dropdown somewhere top right with a "Rerun job with SSH" option.

bsipocz · 2019-10-15T17:56:34Z

or in the workflows menu, there is a "rerun from failed" one.

bsipocz · 2019-10-15T17:57:35Z

(but then these connection related failures seem to be around all morning, seen the same on numpy an hour or so ago).

mhvk · 2019-10-15T18:00:57Z

Ah, logging in and out of circle-ci made that option possible (previously it said I had no write access). THanks!

mhvk · 2019-10-15T18:02:14Z

Oops, didn't mean to start all circle-ci jobs again...

bsipocz · 2019-10-15T18:08:51Z

(circleCI failure is due to this: https://status.docker.com/)

eteq

A few in-line suggestions and some bigger ones here. I'll aim to do another review later when we have #9329 in after you've rebased.

I'm not convinced this belongs in astropy.utils.iers. I think users may want to do updates of leap seconds using this table even independent of IERS. So perhaps it should instead either go in astropy.utils or even astropy.time?
We should add some narrative docs about this, although that could be a follow-on PR since it isn't feature-freeze critical. (I'm also adding the what's new tag to this because I think it's worth highlighting as long as we have the docs for it).
I know this was discussed in #9334 so maybe I missed something in that thread, but... It looks to me like the reason to use the leap second files is because the historical data are needed. Can't we use the current IERS files plus the builtin file to get the latest leap second? That will be wrong if there's multiple intervening leap seconds, but maybe the user doesn't care that much?

eteq · 2019-10-18T16:40:06Z

+    The table should hold columns 'year', 'month', 'tai_utc'.
+
+    Methods are provided to initialize the table from IERS ``Leap_Second.dat``,
+    IETF/ntp ``leap-seconds.list``, or built-in ERFA/SOFA.


The seems like a likely gotcha for potential users...

Suggested change

IETF/ntp ``leap-seconds.list``, or built-in ERFA/SOFA.

IETF/ntp ``leap-seconds.list``, or built-in ERFA/SOFA. Note that to

actually update the leap seconds used by ``astropy``, the

`update_erfa_leap_seconds` method must be explicitly called.

I suppose update_erfa_leap_seconds needs double backticks or full API path.

Done a bit differently, without an explicit name. Could rewrite this part to enumerate all class methods, but that seemed excessive.

mhvk · 2019-10-18T18:10:49Z

@eteq - on the larger picture: the leap seconds are for a number of time formats including the bog-standard ISO ones, as well as for the critical UTC<->TAI conversion; in contrast, the full IERS A/B files are only needed for UT1 and some coordinate transforms, i.e., in much general use they will never be loaded. To have them be loaded just for leap seconds thus is a bit of an unnecessary overhead, especially given that most computers (even windows now) do have leap second tables stored locally already.

As for the location: since IERS is the organisation responsible for leap seconds, I think it does belong in an iers module (though it could be outside of iers.py. It does bring up a question brought up by @astrofrog whether that iers module still belongs in utils...

mhvk · 2019-10-18T21:24:22Z

@eteq - about being part of iers.py: I have wondered about now effectively always pulling in all of astropy.table just for loading time - but it did seem a bit silly to write a light-weight table-like class just for this. My sense is to leave this for later...

mhvk · 2019-10-24T19:09:22Z

@eteq - per your request, I now also allow full override of the built-in erfa table, by another special string for initialize_erfa='empty'. I think this is really not something people should use, so prefer this over adding an extra method.

I think this is done apart from your wish for more documentation. For that, I'm not so sure there is a real need - this is the kind of thing almost nobody will ever need, and, if they need it, they better look at the code anyway.

bsipocz · 2019-10-24T19:48:28Z

I don't see any logical explanation of the travis test failures. Locally I don't see those test collection errors, so will just restart the travis job for now.

mhvk · 2019-10-26T23:59:37Z

@eteq - do you want to have a last look? See #9365 (comment) for an overview of the changes (mostly location, code very similar).

eteq · 2019-10-27T04:35:58Z

@mhvk - I have a few remaining quibbles but since it's definitely better to get this in even without my quibbles addressed than not at off (and we're really in the cutoff time...) I think it's better to merge these and I can put my quibbles in a separate PR. So merging now!

taldcroft · 2019-10-27T11:00:35Z

In a clean build of master right now I'm seeing this:

In [1]: from astropy.io import ascii
WARNING: leap-second auto-update failed due to the following exception: ImportError("cannot import name 'convert_numpy'",) [astropy.time.core]

???

mhvk · 2019-10-27T14:46:33Z

Darn!! There I thought I had finally solved the import order problems. New issue at #9490 (any suggestions most welcome...

bsipocz · 2019-12-07T19:10:11Z

Given that we've upgraded the bundled erfa to be a released version, the second changelog entry here is not needed any longer, so I removed in for the release (but kept the first part).

mhvk · 2019-12-07T19:58:58Z

Sure, thanks!

mhvk added time utils Enhancement erfa labels Oct 14, 2019

mhvk added this to the v4.0 milestone Oct 14, 2019

mhvk requested review from eteq and taldcroft October 14, 2019 21:57

mhvk mentioned this pull request Oct 14, 2019

Update ERFA leap seconds using IERS table. #9334

Closed

mhvk force-pushed the iers-leap-second-list branch 5 times, most recently from f69eaeb to 7dce483 Compare October 15, 2019 01:45

bsipocz reviewed Oct 15, 2019

View reviewed changes

Comment thread astropy/utils/iers/iers.py Outdated

eteq reviewed Oct 18, 2019

View reviewed changes

mhvk force-pushed the iers-leap-second-list branch from eb43147 to e41c891 Compare October 18, 2019 21:20

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

mhvk added 14 commits October 26, 2019 17:57

Add auto-opener.

df555f2

small touch-ups

74a6be4

Changelog entry and minimal documentation

24bbd14

Changelog entry for auto-update of ERFA leap seconds.

71ef88a

Address 32-bit failure

6e4b5c8

Ensure leap seconds are up to date in travis.

7bfc65a

Docs fixes [docs only]

c569cb5

Allow retrieval of built-in ERFA table; doc updates.

ff12cc2

Allow full override of ERFA leap-second table.

3fe508c

Give time an update_leap_seconds function.

2cd9a22

Local import of Time to fix import ordering problems.

ba7188b

Validate expires in erfa.leap_seconds.

dca71cd

Give exception repr rather than str

b5e73d9

Switch to using Time instead of datetime in LeapSeconds

a49c26d

mhvk force-pushed the iers-leap-second-list branch from b22eed0 to 93700d5 Compare October 26, 2019 21:57

Fix links in docstring.

87226dc

mhvk force-pushed the iers-leap-second-list branch from 93700d5 to 87226dc Compare October 26, 2019 23:07

eteq merged commit d853a23 into astropy:master Oct 27, 2019

mhvk mentioned this pull request Oct 27, 2019

Ordering of leap second update can lead to import warnings #9490

Closed

mhvk deleted the iers-leap-second-list branch October 27, 2019 14:54

astrofrog mentioned this pull request Oct 30, 2019

Support programatic leap seconds #5783

Closed

astrofrog mentioned this pull request Nov 7, 2019

Significant increase in import time for several sub-packages #9555

Open

bsipocz added no-changelog-entry-needed and removed no-changelog-entry-needed labels Dec 7, 2019

-    IETF/ntp ``leap-seconds.list``, or built-in ERFA/SOFA.
+    IETF/ntp ``leap-seconds.list``, or built-in ERFA/SOFA. Note that to
+    actually update the leap seconds used by ``astropy``, the
+    `update_erfa_leap_seconds` method must be explicitly called.

Conversation

mhvk commented Oct 14, 2019

Uh oh!

mhvk commented Oct 14, 2019

Uh oh!

Uh oh!

mhvk commented Oct 15, 2019

Uh oh!

bsipocz commented Oct 15, 2019

Uh oh!

bsipocz commented Oct 15, 2019

Uh oh!

bsipocz commented Oct 15, 2019

Uh oh!

mhvk commented Oct 15, 2019

Uh oh!

mhvk commented Oct 15, 2019

Uh oh!

bsipocz commented Oct 15, 2019

Uh oh!

eteq left a comment

Choose a reason for hiding this comment

Uh oh!

eteq Oct 18, 2019

Choose a reason for hiding this comment

Uh oh!

bsipocz Oct 18, 2019

Choose a reason for hiding this comment

Uh oh!

mhvk Oct 18, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhvk commented Oct 18, 2019

Uh oh!

mhvk commented Oct 18, 2019

Uh oh!

mhvk commented Oct 24, 2019

Uh oh!

bsipocz commented Oct 24, 2019

Uh oh!

mhvk commented Oct 26, 2019

Uh oh!

eteq commented Oct 27, 2019

Uh oh!

taldcroft commented Oct 27, 2019

Uh oh!

mhvk commented Oct 27, 2019

Uh oh!

bsipocz commented Dec 7, 2019

Uh oh!

mhvk commented Dec 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants