Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-7587

Add timesys argument to DateTime constructor to toString (implement RFC-219)

    Details

      Description

      Implement RFC-219 as follows:

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

      Require the timescale argument, unless it proves to be a major headache by affecting too much code, in which case default to UTC for backwards compatibility.

      Additional tests that verify that "23:59:60" strings are not produced by toString(TAI) at actual leap seconds.

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment -

            I also found and fixed a minor bug: TT_MINUS_TAI_NSECS was defined as a double instead of long long, causing a loss of precision when converting between TT and TAI.

            Show
            rowen Russell Owen added a comment - I also found and fixed a minor bug: TT_MINUS_TAI_NSECS was defined as a double instead of long long , causing a loss of precision when converting between TT and TAI.
            Hide
            rowen Russell Owen added a comment -

            Implemented as follows:

            • Added a time system argument to DateTime's ISO string constructor and to its toString, gmtime, timeval and timespec methods (the change to gmtime was necessary to support toString, and the remaining methods seemed like an obvious analogy). This argument has no default. Added unit tests.
            • Fixed a roundoff error in computing TT-TAI (TT_MINUS_TAI_NSEC was being needlessly cast to a double) and added a test.
            • Test all allowed separators for ISO strings (a missing test).

            Many packages required minor updates. In the process I found that much of the code using toString was assuming that if a DateTime was defined as TAI then toString would return a TAI implementation, whereas it always wrote UTC. The main issue is that MID-DATE is actually UTC, not the advertised TAI, in all our FITS files, and this will be fixed as part of DM-5503 by no longer writing MID-DATE (though we will still read it, for backwards compatibility), replacing it with a standard FITS keyword instead.

            Show
            rowen Russell Owen added a comment - Implemented as follows: Added a time system argument to DateTime 's ISO string constructor and to its toString , gmtime , timeval and timespec methods (the change to gmtime was necessary to support toString , and the remaining methods seemed like an obvious analogy). This argument has no default. Added unit tests. Fixed a roundoff error in computing TT-TAI ( TT_MINUS_TAI_NSEC was being needlessly cast to a double ) and added a test. Test all allowed separators for ISO strings (a missing test). Many packages required minor updates. In the process I found that much of the code using toString was assuming that if a DateTime was defined as TAI then toString would return a TAI implementation, whereas it always wrote UTC. The main issue is that MID-DATE is actually UTC, not the advertised TAI, in all our FITS files, and this will be fixed as part of DM-5503 by no longer writing MID-DATE (though we will still read it, for backwards compatibility), replacing it with a standard FITS keyword instead.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Looks good. Minor comments on PR.
            Seeing the syntax in Python I don't really like it. I find dt.toString(Datetime.UTC) not very Pythonic. But I guess this was part of the RFC.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Looks good. Minor comments on PR. Seeing the syntax in Python I don't really like it. I find dt.toString(Datetime.UTC) not very Pythonic. But I guess this was part of the RFC.
            Hide
            rowen Russell Owen added a comment -

            Thank you for the helpful and quick review. I have adopted most of your suggestions (though am sticking with making the time system argument have no default value, as requested by Kian-Tat Lim, because I think the existing default value of UTC was leading to bugs).

            Regarding the Python issue: dt.tosString(dt.UTC) also works and I updated the unit test accordingly.

            I think the safest way to support a default for time system is to change DateTime to have a preferred time system that is saved internally, e.g. if a DateTime iscreated as a TAI then time system defaults to TAI for output. I believe the code that incorrect assumed toString returned TAI used that mental model. But it would be a radical change to the API that would require a new RFC.

            Show
            rowen Russell Owen added a comment - Thank you for the helpful and quick review. I have adopted most of your suggestions (though am sticking with making the time system argument have no default value, as requested by Kian-Tat Lim , because I think the existing default value of UTC was leading to bugs). Regarding the Python issue: dt.tosString(dt.UTC) also works and I updated the unit test accordingly. I think the safest way to support a default for time system is to change DateTime to have a preferred time system that is saved internally, e.g. if a DateTime iscreated as a TAI then time system defaults to TAI for output. I believe the code that incorrect assumed toString returned TAI used that mental model. But it would be a radical change to the API that would require a new RFC.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel