Added LOCAL time scale #7122

vermashresth · 2018-01-26T14:06:43Z

issue #6487
Added the ability to use local time scale for Time and TimeDelta. Operations that are valid now

Subtracting two Time instances of 'local' or None type.
Subtracting/Adding Time and TimeDelta instances of local or None type.
local type Time or TimeDelta can only be converted to/from local type, all other types are invalid.

Also made changes to existing tests, because not all transformations between Time are valid now.

astropy-bot · 2018-01-26T14:06:45Z

Hi there @vermashresth 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

mhvk

Thanks for the PR. This looks good, modulo some fairly minor initial comments. Beyond that:

Obviously you need to figure out why the tests failed (sorry, don't have time to look into it myself now). For this, if you haven't done so already, do set up your own development environment.
Once things are working, we'd need some tests with the local timescale. This should include transformations to different formats (which may need some thought for those that have a reference epoch; failing those is an option, but it should be done gracefully).
And once that is done, some added documentation and an entry in CHANGES.rst
Finally, but this can be a separate PR, we should use this for io.fits!

mhvk · 2018-01-26T15:28:51Z

astropy/time/core.py

@@ -30,11 +30,14 @@
 from .formats import TimeFromEpoch  # pylint: disable=W0611


-__all__ = ['Time', 'TimeDelta', 'TIME_SCALES', 'TIME_DELTA_SCALES',
+__all__ = ['Time', 'TimeDelta', 'TIME_SCALES','TIME_TYPES', 'TIME_DELTA_SCALES',


No need to export TIME_TYPES

mhvk · 2018-01-26T15:34:42Z

astropy/time/core.py

            # remove attributes that are invalidated by changing time
            for attr in ('_delta_ut1_utc', '_delta_tdb_tt'):
                if hasattr(out, attr):
                    delattr(out, attr)

        else:  # T - T
+
+            # the scales should be compatible (e.g., cannot convert TDB to LOCAL)
+            if(self.scale is not None and self.scale not in other.SCALES or


For Time, it should not be possible for self.scale or other.scale to be None. Also, the sets are symmetric, so one only needs to check that other.scale in self.SCALES.

mhvk · 2018-01-26T15:37:24Z

astropy/time/tests/test_basic.py

@@ -11,7 +11,7 @@

 from ...tests.helper import catch_warnings
 from ...utils import isiterable
-from .. import Time, ScaleValueError, TIME_SCALES, TimeString, TimezoneInfo
+from .. import Time, ScaleValueError, TIME_SCALES, TIME_TYPES, TimeString, TimezoneInfo


As noted, I think TIME_TYPES should not be exported (since TIME_DELTA_TYPES is not either). Also, I think the tests are clearer if they do not use TIME_TYPES['tt'] but rather STANDARD_TIME_SCALES. So,

from ..core import STANDARD_TIME_SCALES

mhvk · 2018-01-26T15:40:03Z

astropy/time/tests/test_basic.py

@@ -317,10 +317,10 @@ def test_all_transforms(self):
        except reversibility [#2074]"""
        lat = 19.48125
        lon = -155.933222
-        for scale1 in TIME_SCALES:
+        for scale1 in TIME_TYPES['tt']:


use STANDARD_TIME_SCALES here and below

mhvk · 2018-01-26T15:40:15Z

astropy/time/tests/test_delta.py

@@ -7,7 +7,7 @@
 import pytest

 from .. import (Time, TimeDelta, OperandTypeError, ScaleValueError,
-                TIME_SCALES, TIME_DELTA_SCALES)
+                TIME_SCALES,TIME_TYPES, TIME_DELTA_SCALES)


Same comment as above.

mhvk · 2018-01-26T16:09:30Z

@vermashresth - now see your comment in #6487 - Above, I suggested importing STANDARD_TIME_SCALES and use that throughout. I still think that makes most sense, though I now see that it breaks some table tests as well as some documentation. For the latter, we probably should do

TIME_SCALES = STANDARD_TIME_SCALES + LOCAL_SCALES

and similarly for TIME_DELTA_SCALES. This will ensure the order is preserved, which is much easier for doctests.

For the table tests, the failures are related to a worry I expressed above: for formats with a reference time, it is assumed that the time passed in can be transformed to the scale of that reference time. For "local", this is not possible, and hence the format conversion fails. I think in principle, it may be possible to get those conversions to work, but as a first go, I think it is fine to edit the table test to work with STANDARD_TIME_SCALES and then add another test that uses a format that is safe for "local".

vermashresth · 2018-01-26T18:53:38Z

Thanks for the review! @mhvk
I will soon make the changes.

vermashresth · 2018-02-04T21:23:23Z

Hi! @mhvk
Sorry for the delay, had exams going on.
I have fixed the tests in table io.fits and doctests and now the build passes. Regarding the io.fits tests (here) it checks whether a warning is shown when io.fits local time scale is used but now local is supported natively so no warning will be shown. For now, I have removed the checking for warning part but should we get away completely with test for local in there?
Will add tests for local next.

vermashresth · 2018-02-09T20:59:50Z

Hi @mhvk
I have added the tests. Build tests are also passing. Can you please review the changes?

mhvk

This looks very good overall, so I've started to get to pep8 nitpicks... But more importantly, could you add a few notes to the documentation? Both of for Time itself and for the related fits stuff.

Also, it seems time to ask @taldcroft to have a bit of a look.

mhvk · 2018-02-14T12:26:11Z

astropy/time/core.py

-                out._set_scale(other.scale if other.scale is not None
-                               else 'tai')
+                if other.scale is not None and self.scale not in TIME_TYPES[other.scale]:
+                    raise TypeError("Cannot subtract Time and TimeDelta instances with scales '{0}' and '{1}'".format(self.scale, other.scale))


This is a very long line. Could you break it up?

mhvk · 2018-02-14T12:26:37Z

astropy/time/core.py

            # remove attributes that are invalidated by changing time
            for attr in ('_delta_ut1_utc', '_delta_tdb_tt'):
                if hasattr(out, attr):
                    delattr(out, attr)

        else:  # T - T
+
+            # the scales should be compatible (e.g., cannot convert TDB to LOCAL)
+            if(other.scale not in self.SCALES):


No reason for the parentheses, nor really for the black line (since there is a comment)

mhvk · 2018-02-14T12:27:41Z

astropy/time/core.py

@@ -1386,8 +1396,11 @@ def __add__(self, other):
            if other.scale not in (out.scale, None):
                other = getattr(other, out.scale)
        else:
-            out._set_scale(other.scale if other.scale is not None else 'tai')
-
+            if other.scale is not None and self.scale not in TIME_TYPES[other.scale]:


Same comment on the flow; just start with if other.scale is None (as above).

mhvk · 2018-02-14T12:29:56Z

astropy/time/core.py

-                               else 'tai')
+                if other.scale is not None and self.scale not in TIME_TYPES[other.scale]:
+                    raise TypeError("Cannot subtract Time and TimeDelta instances with scales '{0}' and '{1}'".format(self.scale, other.scale))
+                else:


I think the logic is a bit odd here with an implicitly nested if. I would do

if other.scale is None: out._set_scale('tai') else: if self.scale not in TIME_TYPES[other.scale]: raise ... self._set_scale(other.scale)

mhvk · 2018-02-14T12:31:21Z

astropy/time/formats.py

+                tm = getattr(parent, self.epoch_scale)
+            except Exception as err:
+                raise ScaleValueError("Cannot convert from '{0}' epoch scale '{1}'"
+                                  "to specified scale '{2}', got error:\n{3}"


Please align properly.

mhvk · 2018-02-14T12:34:03Z

astropy/time/tests/test_basic.py

+        ScalevalueError
+        """
+        t = Time('2006-01-15 21:24:37.5', scale='local')
+        assert t.jd == 2453751.3921006946


I worry this may (start to) fail due to rounding errors on some platforms. Maybe

assert_allclose(t.jd, 2453751.3921006946, atol=0.001/3600./24., rtol=0.)

mhvk · 2018-02-14T12:34:27Z

astropy/time/tests/test_basic.py

+        t = Time('2006-01-15 21:24:37.5', scale='local')
+        assert t.jd == 2453751.3921006946
+        assert t.mjd == 53750.892100694444
+        assert t.decimalyear == 2006.0408002758752


For the year, atol would need to be further divided by 365.

mhvk · 2018-02-14T12:35:46Z

astropy/time/tests/test_delta.py

+        I.e., time differences of two times should have the scale of the
+        first time.
+
+        There are no local timescales for which this does not work.


This seems a silly statement, since there is only one!

mhvk · 2018-02-14T12:36:58Z

astropy/time/tests/test_delta.py

+        assert t1_recover_t2_scale.scale == scale2
+        t1_recover = getattr(t1_recover_t2_scale, scale1)
+        assert allclose_jd(t1_recover.jd, t1.jd)
+        t2_recover_t1_scale = t1 - dt


Here, also add a test with a TimeDelta(..., scale=None) as well as just subtracting a quantity with time units.

mhvk · 2018-02-14T12:39:14Z

docs/time/index.rst

@@ -264,7 +264,7 @@ both" [#]_. See also [#]_ and [#]_.
 ::

  >>> Time.SCALES
-  ('tai', 'tcb', 'tcg', 'tdb', 'tt', 'ut1', 'utc')
+  ('tai', 'tcb', 'tcg', 'tdb', 'tt', 'ut1', 'utc', 'local')


You should also adjust the text so that it becomes clear what 'local' is, maybe including the table below.

vermashresth · 2018-03-02T07:25:35Z

Hi @mhvk
I have made the changes you requested but the build is now failing and I can't figure out why. It shows some unrelated numpy related errors (and I noticed the same tests are failing in other PRs too). Can you please take a look?
Also, should we update the time scale conversion diagram in documentation?

mhvk

@vermashresth - I made another review, mostly nitpicks and tiny details, and a bit more on the documentation. As noted inline, I don't think we have to change the graph of the scale transformations, but we should note that local is excluded from it.

mhvk · 2018-03-02T14:39:05Z

astropy/table/tests/test_mixin.py

+@pytest.mark.parametrize('table_types', (Table, QTable))
+def test_io_time_write_fits_local(tmpdir, table_types):
+    """
+    Test that table with Time mixin columns can be written by io.fits.


I think the docstring was just copy-pasted from the function above. Could you update it? (i.e., state that this tests the local time scale, and describe why it cannot be included above)?

mhvk · 2018-03-02T14:43:16Z

astropy/time/tests/test_delta.py

@@ -268,15 +269,15 @@ def test_delta_scales_definition(self):
            TimeDelta([0., 1., 10.], format='sec', scale='utc')

    @pytest.mark.parametrize(('scale1', 'scale2'),
-                             list(itertools.product(TIME_SCALES, TIME_SCALES)))
-    def test_scales_for_time_minus_time(self, scale1, scale2):
+                             list(itertools.product(STANDARD_TIME_SCALES, STANDARD_TIME_SCALES)))


nitpick: could you break the line? It is rather long now.

mhvk · 2018-03-02T14:43:50Z

astropy/time/tests/test_delta.py

@@ -297,6 +298,45 @@ def test_scales_for_time_minus_time(self, scale1, scale2):
        t2_recover = getattr(t2_recover_t1_scale, scale2)
        assert allclose_jd(t2_recover.jd, t2.jd)

+    def test_local_scales_for_time_minus_time(self):
+        """T(X) - T2(Y)  -- does T(X) - T2(Y).X and return dT(X)


Please again update the docstring to reflect what this test does.

mhvk · 2018-03-02T14:46:00Z

astropy/time/tests/test_delta.py

+        Also tests that subtracting two time scales, one of which is
+        different from local time scale should raise TypeError.
+        """
+        scale1 = 'local'


Rather than copy the test from above, can we just write 'local'? That makes the test easier to understand, and we're not iterating over different scales here.

mhvk · 2018-03-02T14:47:05Z

astropy/time/tests/test_delta.py

+        # now check with delta time; also check reversibility
+        t1_recover_t2_scale = t2 + dt
+        assert t1_recover_t2_scale.scale == scale2
+        t1_recover = getattr(t1_recover_t2_scale, scale1)


This is a pointless operation when the scale is already 'local'; just call the above t1_recover instead of t1_recover_t2_scale.

mhvk · 2018-03-02T14:51:29Z

docs/time/index.rst

@@ -169,8 +169,8 @@ yday         :class:`~astropy.time.TimeYearDayTime`             2000:001:00:00:0
 ===========  =================================================  ==============================

 .. note:: The :class:`~astropy.time.TimeFITS` format allows for most
-   but not all of the the FITS standard [#]_. Not implemented (yet) is
-   support for a ``LOCAL`` timescale. Furthermore, FITS supports some deprecated
+   but not all of the the FITS standard [#]_. It also supports the


Maybe

.. note:: The :class:`~astropy.time.TimeFITS` format implements most of the FITS standard [#]_, including support for the ``LOCAL`` timescale. Note, though, that FITS supports some deprecated

mhvk · 2018-03-02T14:53:49Z

docs/time/index.rst

@@ -276,13 +276,18 @@ tdb    Barycentric Dynamical Time  (TDB)
 tt     Terrestrial Time            (TT)
 ut1    Universal Time              (UT1)
 utc    Coordinated Universal Time  (UTC)
+local  Local Time Scale            (LOCAL)


Since this is typeset with the (...) part next to the text, maybe just right-adjust (LOCAL) to the end of the column (and move (TT) out by one space)

mhvk · 2018-03-02T14:54:00Z

docs/time/index.rst

 ====== =================================
-
+  


Delete extraneous spaces.

mhvk · 2018-03-02T14:58:51Z

docs/time/index.rst

 .. [#] Wikipedia `time standard <https://en.wikipedia.org/wiki/Time_standard>`_ article
 .. [#] SOFA Time Scale and Calendar Tools
       `(PDF) <http://www.iausofa.org/sofa_ts_c.pdf>`_
 .. [#] `<http://www.ucolick.org/~sla/leapsecs/timescales.html>`_

+.. note:: ``local`` TimeScale is a scale that cannot be converted to/from any


Maybe nice to tell what it is meant for. E.g,

.. note:: the ``local`` time scale is meant for free-running clocks or simulation times, i.e., to represent a time without properly defined scale. It this cannot be converted to any other scale, and arithmetic is possible only with |Time| instances with scale ``local`` and with |TimeDelta| instances with scale ``local`` or `None`.

any other timescale)

mhvk · 2018-03-02T15:03:02Z

docs/time/index.rst

+.. note:: ``local`` TimeScale is a scale that cannot be converted to/from any
+   other scale and to which only TimeDelta with ``None`` type scale could be
+   added or subtracted.
+
 The system of transformation between supported time scales is shown in the


I think maybe here note again that local is excluded; then we do not have to update the figure. E.g.,

The system of transformations between the standard time scales (i.e., all but ``local``) is shown in the ...

vermashresth · 2018-03-03T19:25:37Z

Done :)
Time for entry into CHANGES.rst?

mhvk · 2018-03-04T18:56:48Z

Indeed, time for a changelog entry!

vermashresth · 2018-03-09T18:04:57Z

@mhvk Is there any more work needed on this?
And can you tell more details about using this in io.fits like you suggested. I would like to try implementing that too.

mhvk · 2018-03-09T18:21:52Z

@vermashresth - I think it is all OK, but would like to have a last look. Unfortunately, haven't had time to do so yet. As for the comment about io.fits, you ended up doing that in the PR already: things will now properly translate to and from fits!

mhvk · 2018-04-25T14:47:06Z

@vermashresth - I finally got around to a final review of your nice PR. To ensure we can merge it, I went ahead and just made a commit with some further small changes. I'll now merge, assuming tests pass.

mhvk · 2018-04-25T14:48:30Z

OK, I guess I need to rebase as well so we can get the changelog properly set up... Will do that.

mhvk · 2018-04-25T15:32:16Z

OK, merged. Thanks, @vermashresth!

vermashresth · 2018-04-25T20:29:48Z

Finally :D
Thanks!! @mhvk

bsipocz requested a review from mhvk January 26, 2018 14:10

bsipocz added the time label Jan 26, 2018

bsipocz added this to the v3.1 milestone Jan 26, 2018

mhvk reviewed Jan 26, 2018

View reviewed changes

mhvk mentioned this pull request Jan 26, 2018

Consider adding a Time scale of LOCAL (or None) #6487

Closed

vermashresth force-pushed the issue6487 branch from 9e90960 to 4528e91 Compare February 4, 2018 20:41

mhvk reviewed Feb 14, 2018

View reviewed changes

mhvk reviewed Mar 2, 2018

View reviewed changes

mhvk added the Enhancement label Apr 25, 2018

vermashresth and others added 7 commits April 25, 2018 10:48

added time scale of LOCAL

e56723f

modified tests for LOCAL time scale

bd31d48

added tests for LOCAL scale

ec73c88

pep8 changes and documentation

23e977b

changes in documentation

40d5f77

change log entry

3dd379f

Bits of cleanup of the local time scale tests; change-log entry.

cd84139

mhvk force-pushed the issue6487 branch from 157369b to cd84139 Compare April 25, 2018 14:52

mhvk approved these changes Apr 25, 2018

View reviewed changes

mhvk merged commit 9a5e356 into astropy:master Apr 25, 2018

Added LOCAL time scale #7122

Added LOCAL time scale #7122

Conversation

vermashresth commented Jan 26, 2018

astropy-bot bot commented Jan 26, 2018 • edited

mhvk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhvk commented Jan 26, 2018

vermashresth commented Jan 26, 2018

vermashresth commented Feb 4, 2018

vermashresth commented Feb 9, 2018

mhvk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vermashresth commented Mar 2, 2018

mhvk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vermashresth commented Mar 3, 2018

mhvk commented Mar 4, 2018

vermashresth commented Mar 9, 2018

mhvk commented Mar 9, 2018

mhvk commented Apr 25, 2018

mhvk commented Apr 25, 2018

mhvk commented Apr 25, 2018

vermashresth commented Apr 25, 2018

astropy-bot bot commented Jan 26, 2018 •

edited