Expose reading and setting of the ERFA leap second table by mhvk · Pull Request #9329 · astropy/astropy

mhvk · 2019-10-06T21:49:08Z

@eteq - this is step 1 to using/updating the leap second table. I went a bit beyond @jwoillez's implementation by also allowing one to retrieve the current leap second table, using a new eraGetLeapSeconds. With that, it seemed it would be better to also rename eraUpdateLeapSeconds to eraSetLeapSeconds.

One can now retrieve these as structured arrays using ~~_erfa.get_leap_second_table and _erfa.set_leap_second_table.~~ EDIT (name change): erfa.get_leap_seconds and _erfa.set_leap_seconds.

EDIT 2: I hid the C functions behind a new leap_second singleton, with get, set, and update methods.

mhvk · 2019-10-07T15:07:56Z

astropy/_erfa/core.py.templ

 from astropy.utils.misc import check_broadcast

 from . import ufunc
-from .ufunc import (dt_eraASTROM, dt_eraLDBODY, dt_pv,


Removed imports here since they are not used anyway, and make much more sense in the _erfa module.

astropy/_erfa/erfa_generator.py

mhvk · 2019-10-09T01:26:34Z

@eteq - one possible issue with the whole procedure is that the ERFA update is, per definition, global, ie., it will affect anything else that has the library in memory (sub-process, etc.). Similarly, the module is not sub-process safe. Of course, this is an issue only if different subprocesses had different wishes for the leap-second table, which is arguably very unlikely. I guess in principle it might be better instead to offer a hook in eraDat that calls out to get the leap-second table (which then by default returns the built-in one, but can be overridden somehow).

Anyway, my own sense is to punt on this for now, and leave this for 5.0 or so.

mhvk · 2019-10-12T15:32:08Z

@eteq - yet another issue: if we go for an astropy-only approach on this for 4.0, it will not work for any distribution that separately ships liberfa, since in those we cannot actually override the leap second table. Since I work on such a distribution myself, this is a problem..

Or would it work for those to compile just dat.c ourselves?

mhvk · 2019-10-14T13:27:44Z

@eteq - would you be able to review? For now, I decided to forge ahead - we can worry about the distribution aspect later.

mhvk · 2019-10-14T13:31:14Z

astropy/_erfa/interface.py

+"""
+Interfaces to the ERFA library, in particular for leap seconds.
+"""
+from datetime import datetime, timedelta


It would be nice to use Time but that leads to round-trip issues when adding auto-updates upon loading of _erfa.

None of this is actually instantiated at import time, right? So can't we just use time imports inside the functions? I think that's a more consistent approach.

(If I missed an import-time requirement, then you can ignore this, though. It's not that bad since it's easy to transition between Time and datetime anyway)

I wish!! (and tried). The problem is that Time initiation needs erfa installed (even the plainest formats like 'iso' rely on it). More specifically, Time.now() needs to have the leap seconds up to date (in case 'now' is on a leap-second day), yet here I need to have Time.now() accessible to tell whether to update the leap-second table.

eteq

I think this is overall the right direction, @mhvk. I think we don't need to worry a whole lot about the interface since nominally this is all "private" so we can iterate on it and not worry too much about backwards compatibility. So if #9365 + this is ready and we're bumping up against feature freeze, I'm ok with merging this without worrying a ton about some of these items and sort it out later.

But two broad issues to bring up in addition to my inline comments:

Can the changes to dat go in a separate file? this would probably require adding it to erfaextras.h or something, but I think that will be easier to manage than the dat.c changes. My worry being this is a very substansive change and easy to screw up the "bring in a new erfa version process" if it's mixed in dat.c whereas if we did dat_extras.c or similar then we can just leave that file alone during the update process.
The class level singleton seems a bit odd to me. Why use that approach instead of either a singleton instance (which I imagine is basically just a find-and-replace), vs standard module functions with globals as the shared state?

eteq · 2019-10-18T16:10:57Z

astropy/_erfa/interface.py

@@ -0,0 +1,177 @@
+# Licensed under a 3-clause BSD style license - see LICENSE.rst
+"""
+Interfaces to the ERFA library, in particular for leap seconds.


I think it's better to make this only about leap seconeds. I.e., something like:

Suggested change

Interfaces to the ERFA library, in particular for leap seconds.

Interfaces to controlling leap seconds in the ERFA library.

and then change the name of the module file from interface.py to leap_seconds.py

astropy/_erfa/erfa_generator.py

eteq · 2019-10-18T16:14:52Z

astropy/_erfa/interface.py

+"""
+Interfaces to the ERFA library, in particular for leap seconds.
+"""
+from datetime import datetime, timedelta


None of this is actually instantiated at import time, right? So can't we just use time imports inside the functions? I think that's a more consistent approach.

(If I missed an import-time requirement, then you can ignore this, though. It's not that bad since it's easy to transition between Time and datetime anyway)

mhvk · 2019-10-18T17:02:03Z

Yes, I've been wondering about making the changes to dat.c more minimal, ideally such that we can use a system library and things still work. But it needs me understand global variables a bit better...

On interfaces vs light_seconds for the module - I had the latter originally, but the problem is that the singleton also is called light_seconds, and from light_seconds import light_seconds then hides the module. Or am I wrong?

mhvk · 2019-10-18T17:03:28Z

p.s. The reason for a singleton class is that I started with ScienceState - which is a singleton class. If you think it is clearer, I can certainly make it a singleton module, i.e., def get(), etc. That would also solve the naming issue...

mhvk · 2019-10-18T20:20:54Z

@eteq - I managed to get out most of the changes to dat.c, into a new erfadatextra.c (prefixed by erfa in analogy with erfaversion.c - I think it makes sense to avoid calling anything not from SOFA by a name that could imply it came from SOFA).

I looked a bit at the module replacement for the current light_second singleton class, but don't like that then I cannot have properties. My sense would be to leave it as is for now, since it is so analogous to ScienceState (and we use singleton classes also in units.formats).

mhvk · 2019-10-18T21:36:45Z

@eteq - from the commits, it may not be so clear, but the total change in dat.c is now quite small: relative to the commit just before that by you of @jwoillez's changes,

git diff ddab2476d477fa4e92b058c4c72e70f05535f31b cextern/erfa/dat.c
 #include "erfa.h"
+#include "erfadatextra.h"
 
 int eraDat(int iy, int im, int id, double fd, double *deltat)
 /*
@@ -144,10 +145,7 @@ int eraDat(int iy, int im, int id, double fd, double *deltat)
    enum { NERA1 = (int) (sizeof drift / sizeof (double) / 2) };
 
 /* Dates and Delta(AT)s */
-   static const struct {
-      int iyear, month;
-      double delat;
-   } changes[] = {
+   static const eraLEAPSECOND _changes[] = {
       { 1960,  1,  1.4178180 },
       { 1961,  1,  1.4228180 },
       { 1961,  8,  1.3728180 },
@@ -193,7 +191,13 @@ int eraDat(int iy, int im, int id, double fd, double *deltat)
    };
 
 /* Number of Delta(AT) changes */
-   enum { NDAT = (int) (sizeof changes / sizeof changes[0]) };
+   enum { _NDAT = (int) (sizeof _changes / sizeof _changes[0]) };
+
+/* Get/initialise leap-second if needed */
+   int NDAT;
+   eraLEAPSECOND *changes;
+
+   NDAT = eraDatini(_changes, _NDAT, &changes);
 
 /* Miscellaneous local variables */

mhvk · 2019-10-24T18:17:49Z

@eteq - this is still the simplest PR of the two - could you re-review?

eteq · 2019-10-25T09:09:04Z

@mhvk - ok in its current state I think this is basically good. I didn't merge though because I wanted to give one more try of suggesting a change from the name interfaces.py to leap_seconds.py (which goes hand-in-hand with #9329 (comment)). I see your point from leap_seconds import leap_seconds is bad, but perhaps just have the file name be leap_second.py? Or change the class to leap_second_table or similar? I don't actually have a strong opinion on the exact wording as long as the module name be a bit more specific than "interfaces".

(But I approved because I think this is good other than that one name change.)

mhvk · 2019-10-25T19:21:13Z

I'm quite happy to change the name, but not to leap_seconds! How about utils or helpers? In a way, the actual interface is in C so this is just a helper module/routine/class.

eteq · 2019-10-25T19:52:39Z

@mhvk - ~~I'm ok with helpers~~, or possibly extras (since we're already using that on the erfa c side to mean "stuff beyond standard erfa bits"). If we do utils I'm 90% sure @adrn will just revert it all 😉

EDIT: after a moments more thought I decided I do prefer extras over helpers, but could live with either.

pllim · 2019-10-25T19:54:58Z

Why not leap first can make it utils anyway? 😏

eteq · 2019-10-25T21:56:37Z

@mhvk - also if it would be helpful I can do some of the clean-up steps to finish some of this and/or #9365 off if you're running out of time today.

mhvk · 2019-10-25T22:09:39Z

@eteq - thanks, may be needed. But right now pushing up a version with helpers.py...

This avoids locking us in to the C routines. Also, the update method is needed to interface with leap-second lists, and is much easier done in python.

mhvk · 2019-10-25T23:36:28Z

coverage failure is a false positive. Since approved before, merging...

mhvk added Enhancement erfa no-changelog-entry-needed labels Oct 6, 2019

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

mhvk commented Oct 7, 2019

View reviewed changes

astropy/_erfa/erfa_generator.py Outdated Show resolved Hide resolved

pllim requested a review from eteq October 7, 2019 15:36

This was referenced Oct 7, 2019

Added a hook to eraDat to update the leap seconds. liberfa/erfa#49

Closed

Update ERFA leap seconds using IERS table. #9334

Closed

mhvk force-pushed the erfa-expose-leap-second-table branch from dd664bb to 07afb61 Compare October 9, 2019 01:10

mhvk force-pushed the erfa-expose-leap-second-table branch from 07afb61 to 9e3055b Compare October 14, 2019 13:25

mhvk commented Oct 14, 2019

View reviewed changes

mhvk force-pushed the erfa-expose-leap-second-table branch from 9e3055b to bf0fcd4 Compare October 14, 2019 13:36

mhvk mentioned this pull request Oct 14, 2019

Update ERFA leap seconds using new IERS LeapSecond class. #9365

Merged

eteq requested changes Oct 18, 2019

View reviewed changes

mhvk force-pushed the erfa-expose-leap-second-table branch from bf0fcd4 to 5dc7813 Compare October 18, 2019 20:17

eteq approved these changes Oct 25, 2019

View reviewed changes

mhvk added 7 commits October 25, 2019 18:10

Also add a way to access the ERFA leap second table.

dc6bbbb

Allow leap second table to be gotten to and set from a structured array.

a9b203c

Import useful stuff from ufunc directly into _erfa rather than in core.

04e24cd

Add tests for the leap second setting/getting.

81782de

Add TODO about protections for multiple interpreters.

5f753c5

Get/set/update leap seconds via a python class.

eab4fdf

This avoids locking us in to the C routines. Also, the update method is needed to interface with leap-second lists, and is much easier done in python.

Change name from interface.py to helpers.py

04a0e83

mhvk force-pushed the erfa-expose-leap-second-table branch from 5dc7813 to 04a0e83 Compare October 25, 2019 22:10

mhvk merged commit b0a84f6 into astropy:master Oct 25, 2019

mhvk deleted the erfa-expose-leap-second-table branch October 25, 2019 23:36

pllim mentioned this pull request Nov 22, 2019

4.0 rc1: astropy uses unreleased version of erfa #9654

Closed

mhvk mentioned this pull request Dec 5, 2019

Leap second access liberfa/erfa#58

Merged

	Interfaces to the ERFA library, in particular for leap seconds.
	Interfaces to controlling leap seconds in the ERFA library.

Uh oh!

Conversation

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

Uh oh!

Uh oh!

mhvk Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhvk commented Oct 9, 2019

Uh oh!

mhvk commented Oct 12, 2019

Uh oh!

mhvk commented Oct 14, 2019

Uh oh!

mhvk Oct 14, 2019

Choose a reason for hiding this comment

Uh oh!

eteq 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!

eteq left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eteq Oct 18, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eteq Oct 18, 2019

Choose a reason for hiding this comment

Uh oh!

mhvk commented Oct 18, 2019

Uh oh!

mhvk commented Oct 18, 2019

Uh oh!

mhvk commented Oct 18, 2019

Uh oh!

mhvk commented Oct 18, 2019

Uh oh!

mhvk commented Oct 24, 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!

pllim commented Oct 25, 2019

Uh oh!

eteq commented Oct 25, 2019

Uh oh!

mhvk commented Oct 25, 2019

Uh oh!

mhvk commented Oct 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mhvk commented Oct 6, 2019 •

edited

Loading

eteq left a comment •

edited

Loading

eteq commented Oct 25, 2019 •

edited

Loading