Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-219

Add support to daf::base::DateTime for TAI strings

    Details

    • Type: RFC
    • Status: Implemented
    • Resolution: Done
    • Component/s: DM
    • Labels:
      None

      Description

      The current lsst::daf::base::DateTime can only read and write ISO8601 strings as UTC. This means it cannot easily be used to read or write FITS header dates, which have the following requirements:

      • TAI dates are supported, by setting TIMESYS=TAI (very useful, since TAI is a uniform time system, so it makes a logical choice for recording dates)
      • FITS ISO8601 dates cannot contain a "Z" (or other time zone character)

      I propose the following modifications:

      • DateTime(std::string const& iso8601) gain a time system argument that defaults to UTC. This is necessary in order to easily and correctly read valid FITS dates, e.g. for massaging raw data into our format.
      • DateTime.toString() gains two arguments:
        • a time system argument that defaults to UTC. This allows us to easily write TAI dates to our FITS files, which is a clear win.
        • a flag that controls whether to output the "Z". This saves the trouble of stripping that character manually, increasing the chances that programmers will write correct FITS dates. However, we can live without it if necessary.

      If accepted, I would like to implement this as part of DM-5503. If rejected then I suggest the following:

      • Always write dates as MJD TAI and never as ISO8601. This has two minor concerns:
        • MJD potentially loses some digits
        • ISO dates have some human readability advantages (though MJD is also nice for other reasons, which is probably why both are often written)
      • Do not support reading raw dates whose dates are ISO8601 TAI. I do not know if this will be a problem for any cameras that we presently support; in many cases dates are written as both MJD and ISO8601, giving us an alternative for reading, and of course many cameras write UTC dates.

        Attachments

          Issue Links

            Activity

            rowen Russell Owen created issue -
            rowen Russell Owen made changes -
            Field Original Value New Value
            Link This issue relates to DM-5503 [ DM-5503 ]
            Hide
            ktl Kian-Tat Lim added a comment -

            I think it's a misnomer to refer to "ISO8601 TAI". There's no such thing. There's a string that happens to look like an ISO8601-formatted date and that is externally indicated as being interpreted in the TAI timescale.

            If we add a timescale argument to the constructor and toString(), I would tend to prefer that it not be defaulted, so that all uses are explicit. In addition, the timescale argument to toString() would control the "Z": never if TAI, always if UTC.

            TAI is indeed a uniform timescale; convolving that simplicity with the messiness of date formatting, including variable-length months and leap days, seems wacky to me.

            Show
            ktl Kian-Tat Lim added a comment - I think it's a misnomer to refer to "ISO8601 TAI". There's no such thing. There's a string that happens to look like an ISO8601-formatted date and that is externally indicated as being interpreted in the TAI timescale. If we add a timescale argument to the constructor and toString() , I would tend to prefer that it not be defaulted, so that all uses are explicit. In addition, the timescale argument to toString() would control the "Z": never if TAI, always if UTC. TAI is indeed a uniform timescale; convolving that simplicity with the messiness of date formatting, including variable-length months and leap days, seems wacky to me.
            Hide
            tjenness Tim Jenness added a comment -

            Leap days are always a problem. daf_base and astropy.time disagree on how to handle the leap second in some conversion (instantly or spread over the day). TAI/UTC to MJD is also fraught as to how many seconds are in the day when you work out the fraction of day.

            Show
            tjenness Tim Jenness added a comment - Leap days are always a problem. daf_base and astropy.time disagree on how to handle the leap second in some conversion (instantly or spread over the day). TAI/UTC to MJD is also fraught as to how many seconds are in the day when you work out the fraction of day.
            Hide
            rowen Russell Owen added a comment -

            Tim Jenness yes ISO strings are tricky near leap seconds. I assume our DateTime object can handle it correctly, but if so, it is an interesting question if the existing code to write an ISO-like string can be safely used to write TAI (and if not, we need to fix it). Which code spreads the leap second out over the day? That is wrong and we need to avoid it.

            Kian-Tat Lim if we write "Z" for UTC and no "Z" for TAI, I can live with that. We'll have to manually strip it when writing FITS, but we don't write enough dates to FITS to worry about it, and I hope we'll always write TAI dates.

            Show
            rowen Russell Owen added a comment - Tim Jenness yes ISO strings are tricky near leap seconds. I assume our DateTime object can handle it correctly, but if so, it is an interesting question if the existing code to write an ISO-like string can be safely used to write TAI (and if not, we need to fix it). Which code spreads the leap second out over the day? That is wrong and we need to avoid it. Kian-Tat Lim if we write "Z" for UTC and no "Z" for TAI, I can live with that. We'll have to manually strip it when writing FITS, but we don't write enough dates to FITS to worry about it, and I hope we'll always write TAI dates.
            Hide
            Parejkoj John Parejko added a comment -

            Going with Tim Jenness's suggestions on HipChat and the Astropy SPIE paper: why don't we use astropy to ingest dates from raw images, save them as TAI internally (type: long long?) and not support any conversion in the LSST stack. If one wants a string or other datetime conversion, they use astropy to convert that TAI.

            Do we actually need to do any non-TAI work at the C++ level in the stack?

            Show
            Parejkoj John Parejko added a comment - Going with Tim Jenness 's suggestions on HipChat and the Astropy SPIE paper: why don't we use astropy to ingest dates from raw images, save them as TAI internally (type: long long?) and not support any conversion in the LSST stack. If one wants a string or other datetime conversion, they use astropy to convert that TAI. Do we actually need to do any non-TAI work at the C++ level in the stack?
            Hide
            ktl Kian-Tat Lim added a comment -

            I'm much more comfortable with the Astropy solution.

            Show
            ktl Kian-Tat Lim added a comment - I'm much more comfortable with the Astropy solution.
            Hide
            rowen Russell Owen added a comment - - edited

            If we go with John Parejko's suggestion then we need a C++ representation for TAI.

            Some possibilities:

            • Use signed 64 bit integer microseconds from the MJD epoch. This gives us enough range to not need to worry about rollover date, and the convenience of signed integers for performing date math. This is my preferred solution.
            • If we need nanosecond precision, one option is to use unsigned 64 bit integer nanoseconds since the MJD epoch. This rolls over at 213503 days, which is year 2443. I worry about picking such a nearby rollover date, and unsigned integers are a hassle for computing differences. Another option is to use the POSIX timestamp struct.

            For LSST the shutter specification is such that we probably can't make use of timing better than milliseconds. However, other cameras may want more.

            We could use a 64-bit float to represent MJD; through roughly 2134 we could get of order 10 microsecond precision, at which point it would drop to 100 microseconds. Floats are very convenient, but the lack of precision bothers me.

            Show
            rowen Russell Owen added a comment - - edited If we go with John Parejko 's suggestion then we need a C++ representation for TAI. Some possibilities: Use signed 64 bit integer microseconds from the MJD epoch. This gives us enough range to not need to worry about rollover date, and the convenience of signed integers for performing date math. This is my preferred solution. If we need nanosecond precision, one option is to use unsigned 64 bit integer nanoseconds since the MJD epoch. This rolls over at 213503 days, which is year 2443. I worry about picking such a nearby rollover date, and unsigned integers are a hassle for computing differences. Another option is to use the POSIX timestamp struct. For LSST the shutter specification is such that we probably can't make use of timing better than milliseconds. However, other cameras may want more. We could use a 64-bit float to represent MJD; through roughly 2134 we could get of order 10 microsecond precision, at which point it would drop to 100 microseconds. Floats are very convenient, but the lack of precision bothers me.
            Hide
            ktl Kian-Tat Lim added a comment -

            I think the proposal for the C++ representation of TAI times is to use DateTime but stripped of all UTC capabilities.

            Show
            ktl Kian-Tat Lim added a comment - I think the proposal for the C++ representation of TAI times is to use DateTime but stripped of all UTC capabilities.
            Hide
            rowen Russell Owen added a comment -

            I don't understand your suggestion. What do we get out of a TAI-only DateTime object? What API does it have? Can it read or write ISO8601 strings?

            Show
            rowen Russell Owen added a comment - I don't understand your suggestion. What do we get out of a TAI-only DateTime object? What API does it have? Can it read or write ISO8601 strings?
            Hide
            tjenness Tim Jenness added a comment -

            DateTime already uses nanoseconds internally.

            Show
            tjenness Tim Jenness added a comment - DateTime already uses nanoseconds internally.
            Hide
            rowen Russell Owen added a comment -

            Yes, DateTime stores the date as nanoseconds since the MJD epoch in a "long long", which is only guaranteed to be 64 bits. In my opinion that is not acceptable for long term use. We should either switch to microseconds or use a more sophisticated internal representation.

            Show
            rowen Russell Owen added a comment - Yes, DateTime stores the date as nanoseconds since the MJD epoch in a "long long", which is only guaranteed to be 64 bits. In my opinion that is not acceptable for long term use. We should either switch to microseconds or use a more sophisticated internal representation.
            Hide
            rowen Russell Owen added a comment - - edited

            The basic issue is that I need some way to read and write FITS standard dates using C++, preferably TAI dates.

            I think the following solutions have been proposed:

            1. Use AST. That sounds great, except it will set back DM-5503 because we don't have AST in the stack. I also have no idea how our FITS metadata and AST can be made to interoperate.
            2. Adopt Tim Jenness's C++ wrapper around AstroPy time.
            3. Add a time system argument to the DateTime constructor and toString method. This assumes DateTime can handle seconds in the day correction for TAI.
            4. Kian-Tat Lim suggested modifying DateTime in some way that I don't understand.
            Show
            rowen Russell Owen added a comment - - edited The basic issue is that I need some way to read and write FITS standard dates using C++, preferably TAI dates. I think the following solutions have been proposed: Use AST. That sounds great, except it will set back DM-5503 because we don't have AST in the stack. I also have no idea how our FITS metadata and AST can be made to interoperate. Adopt Tim Jenness 's C++ wrapper around AstroPy time. Add a time system argument to the DateTime constructor and toString method. This assumes DateTime can handle seconds in the day correction for TAI. Kian-Tat Lim suggested modifying DateTime in some way that I don't understand.
            Hide
            Parejkoj John Parejko added a comment -

            I'm also confused as to why we would maintain our own DateTime object. The three solutions that make sense to me (which share many things with Russell's 2. above) are:

            1. Fully adopt astropy.time as our internal time representation. This is tricky if you want to do things with it at the C++ level, but Tim has a partially implemented solution.
            2. Use astropy.time's internal representation (two doubles), with a few methods to help users lift it into astropy if they want to do conversions, and add the necessary mathematical operations to make it useful for arithmetic. This may be as simple as adding an ERFA dependency, or it may be a fair amount of work.
            3. Use a mathematically simple numeric representation internally (e.g. long long microseconds from MJD epoch) and provide helper functions to produce an astropy.time when people need to do more with it than simple math. We would still have to provide a way to (de)persist dates at the C++ level (possibly via calling up to python as in 1.).

            Alternately, we could use AST internally for dates, as Russell proposes in his 1. I don't know what that would entail, nor how well it plays with astropy, but it's almost certainly much more functionally complete than anything we have time to write.

            Show
            Parejkoj John Parejko added a comment - I'm also confused as to why we would maintain our own DateTime object. The three solutions that make sense to me (which share many things with Russell's 2. above) are: Fully adopt astropy.time as our internal time representation. This is tricky if you want to do things with it at the C++ level, but Tim has a partially implemented solution. Use astropy.time's internal representation (two doubles), with a few methods to help users lift it into astropy if they want to do conversions, and add the necessary mathematical operations to make it useful for arithmetic. This may be as simple as adding an ERFA dependency, or it may be a fair amount of work. Use a mathematically simple numeric representation internally (e.g. long long microseconds from MJD epoch) and provide helper functions to produce an astropy.time when people need to do more with it than simple math. We would still have to provide a way to (de)persist dates at the C++ level (possibly via calling up to python as in 1.). Alternately, we could use AST internally for dates, as Russell proposes in his 1. I don't know what that would entail, nor how well it plays with astropy, but it's almost certainly much more functionally complete than anything we have time to write.
            Hide
            tjenness Tim Jenness added a comment -

            To be clear, I was not suggesting we use AST to replace DateTime. I was suggesting that we use AST to parse the time headers in the files.

            Show
            tjenness Tim Jenness added a comment - To be clear, I was not suggesting we use AST to replace DateTime. I was suggesting that we use AST to parse the time headers in the files.
            Hide
            Parejkoj John Parejko added a comment -

            Tim Jenness Parse the time headers into what though? I'm still not clear why we should have our own internal DateTime object.

            Show
            Parejkoj John Parejko added a comment - Tim Jenness Parse the time headers into what though? I'm still not clear why we should have our own internal DateTime object.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            How about just using std::chrono::time_point and friends?
            As for precision of the time points duration I would opt for nanoseconds. It may just be the radio astronomer in me talking, but I feel anything lower is going to hurt us at some point.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - How about just using std::chrono::time_point and friends? As for precision of the time points duration I would opt for nanoseconds. It may just be the radio astronomer in me talking, but I feel anything lower is going to hurt us at some point.
            Hide
            Parejkoj John Parejko added a comment -

            Thanks for the suggestion Pim Schellart [X]. We'll look into it.

            Simon Krughoff points out that pex_logging depends on daf_base::DateTime, which complicates matters.

            Show
            Parejkoj John Parejko added a comment - Thanks for the suggestion Pim Schellart [X] . We'll look into it. Simon Krughoff points out that pex_logging depends on daf_base::DateTime, which complicates matters.
            Hide
            rowen Russell Owen added a comment - - edited

            I believe we are discussing several unrelated but important topics here.

            The topic I would like to focus on in this RFC is how to read and write FITS compatible TAI dates using C++. This is necessary because VisitInfo contains the date of observation, and is a C++ object that uses C++ for persistence (normally to a FITS header, though like much of the rest of ExposureInfo, it can also be persisted to a table, a feature that is used in coadds).

            For full precision we need to represent FITS dates as ISO8601 strings (FITS also supports MJD as a float, but a 64-bit float doesn't have enough resolution). For sanity we want TAI instead of UTC dates. FITS supports all of this. DateTime does not, because its ISO8601 format is always UTC.

            The minimal change required is that DateTime(string) and DateTime.toString() both gain a time system argument (which can default to UTC, for backwards compatibility). As per @ktl's suggestion, toString will write a "Z" time zone character for UTC and no time zone character for TAI. A subtlety is that the existing ISO writing code may not work correctly for TAI, since it was written for UTC (which can have leap seconds). If this proposal is accepted, I plan to implement it as part of DM-5503.

            There are other issues that I think should be discussed elsewhere (perhaps an RFD), and which are probably interrelated:

            • daf::base::DateTime uses an internal data format that will roll over in too few years. Note that if and when we change it, our table persistence of dates will almost certainly break.
            • We would like interoperability of daf::base::DateTime and astropy.time
            • It is not clear that we need daf::base::DateTime to support any time system except TAI. It would simplify the code to remove support for other time systems.
            Show
            rowen Russell Owen added a comment - - edited I believe we are discussing several unrelated but important topics here. The topic I would like to focus on in this RFC is how to read and write FITS compatible TAI dates using C++. This is necessary because VisitInfo contains the date of observation, and is a C++ object that uses C++ for persistence (normally to a FITS header, though like much of the rest of ExposureInfo, it can also be persisted to a table, a feature that is used in coadds). For full precision we need to represent FITS dates as ISO8601 strings (FITS also supports MJD as a float, but a 64-bit float doesn't have enough resolution). For sanity we want TAI instead of UTC dates. FITS supports all of this. DateTime does not, because its ISO8601 format is always UTC. The minimal change required is that DateTime(string) and DateTime.toString() both gain a time system argument (which can default to UTC, for backwards compatibility). As per @ktl's suggestion, toString will write a "Z" time zone character for UTC and no time zone character for TAI. A subtlety is that the existing ISO writing code may not work correctly for TAI, since it was written for UTC (which can have leap seconds). If this proposal is accepted, I plan to implement it as part of DM-5503 . There are other issues that I think should be discussed elsewhere (perhaps an RFD), and which are probably interrelated: daf::base::DateTime uses an internal data format that will roll over in too few years. Note that if and when we change it, our table persistence of dates will almost certainly break. We would like interoperability of daf::base::DateTime and astropy.time It is not clear that we need daf::base::DateTime to support any time system except TAI. It would simplify the code to remove support for other time systems.
            Hide
            ktl Kian-Tat Lim added a comment -

            I'm OK with a minimal fix for now. That would encompass:

            • DateTime(string, timescale) with "Z" required for UTC and forbidden for TAI. Ideally forbid "23:59:60" leap seconds for TAI as well.
            • DateTime.toString(timescale) with "Z" produced for UTC and not for TAI.
            • Given the "Z" indicator, I'm willing to live with a defaultable timescale, although I'm still a bit worried that having it implicit may lead to confusion.
            • Additional tests that verify that "23:59:60" strings are not produced by toString(TAI) at actual leap seconds.
            • A ticket to fix the UTC constructor and toString which currently fail at leap seconds.
            Show
            ktl Kian-Tat Lim added a comment - I'm OK with a minimal fix for now. That would encompass: DateTime(string, timescale) with "Z" required for UTC and forbidden for TAI. Ideally forbid "23:59:60" leap seconds for TAI as well. DateTime.toString(timescale) with "Z" produced for UTC and not for TAI. Given the "Z" indicator, I'm willing to live with a defaultable timescale, although I'm still a bit worried that having it implicit may lead to confusion. Additional tests that verify that "23:59:60" strings are not produced by toString(TAI) at actual leap seconds. A ticket to fix the UTC constructor and toString which currently fail at leap seconds.
            Hide
            rowen Russell Owen added a comment -

            Adopted as per K-T's design.

            I will also look into making timescale a required argument; if it does not impact too much existing code then I will do that, to avoid confusion.

            Show
            rowen Russell Owen added a comment - Adopted as per K-T's design. I will also look into making timescale a required argument; if it does not impact too much existing code then I will do that, to avoid confusion.
            rowen Russell Owen made changes -
            Resolution Done [ 10000 ]
            Status Proposed [ 10805 ] Adopted [ 10806 ]
            rowen Russell Owen made changes -
            Link This issue is triggering DM-7587 [ DM-7587 ]
            rowen Russell Owen made changes -
            Status Adopted [ 10806 ] Implemented [ 11105 ]
            Hide
            rhl Robert Lupton added a comment -

            Making timescale a required argument broke code (pipe/drivers/constructCalibs.py in v12_1). I'm not sure that it should have been attached to this RFC at the last minute.

            Show
            rhl Robert Lupton added a comment - Making timescale a required argument broke code (pipe/drivers/constructCalibs.py in v12_1). I'm not sure that it should have been attached to this RFC at the last minute.
            Hide
            tjenness Tim Jenness added a comment -

            It was acknowledged that code would break. pipe_drivers is an optional dependency of lsst_distrib by the looks of it.

            Show
            tjenness Tim Jenness added a comment - It was acknowledged that code would break. pipe_drivers is an optional dependency of lsst_distrib by the looks of it.
            Hide
            rhl Robert Lupton added a comment - - edited

            I interpreted "if it does not impact too much code" as either a promise to fix all occurrences, or to RFC the change separately.

            The fact that this the broken code is (currently) optional tells us that the calibration products are falling through the cracks.

            Show
            rhl Robert Lupton added a comment - - edited I interpreted "if it does not impact too much code" as either a promise to fix all occurrences, or to RFC the change separately. The fact that this the broken code is (currently) optional tells us that the calibration products are falling through the cracks.
            Hide
            tjenness Tim Jenness added a comment -

            I believe Russell Owen did try to fix all the code he knew about. Optional code that doesn't run tests that trigger the problem are harder to deal with.

            Show
            tjenness Tim Jenness added a comment - I believe Russell Owen did try to fix all the code he knew about. Optional code that doesn't run tests that trigger the problem are harder to deal with.

              People

              • Assignee:
                rowen Russell Owen
                Reporter:
                rowen Russell Owen
                Watchers:
                John Parejko, Kian-Tat Lim, Pim Schellart [X] (Inactive), Robert Lupton, Russell Owen, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Planned End:

                  Summary Panel