Use integer time throughout codebase #73

spenczar · 2023-10-02T23:10:40Z

This is a big PR. I'm relying very heavily on tests, and there are probably spots that I have missed.

I'll try to highlight areas of special concern.

spenczar · 2023-10-02T23:12:03Z

adam_core/coordinates/cometary.py

-            cartesian.time.to_astropy().tdb.mjd,
+            cartesian.time.to_numpy(),


I've decided to add to_numpy as a convenience method for the conversion to TDD-scaled, MJD-epoched, double-precisioned timestamps. Perhaps the name is opaque?

I think to_numpy is a good addition. Though I agree that the name is a little opaque. Do you think it might make sense to add a scale kwarg that will return the numpy array in the desired scale (to_numpy(scale="tdb"))? The default can be set to its current scale?

Another option (now having read more of the code) might be to enable symmetry between .from_mjd by adding a to_mjd(scale="tdb"). The current mjd() could suffice as well with a scale parameter. The latter could just return a pyarrow array (the additional call to .to_numpy() from there doesn't seem super cumbersome from my perspective).

I suspect a lot of astronomers will expect an interface similar to astropy. Don't know how much we want to mimic it but that's a thought. This doesn't have to be addressed in this PR obviously. Here are some options:

import numpy as np from adam_core.time import Timestamp mjd_tdb = np.arange(59000, 60000) times = Timestamp.from_mjd(mjd_tdb, scale="tdb") # this feels very nice and not much different than astropy mjd_utc = times.utc().mjd().to_numpy() mjd_utc = times.mjd(scale="utc").to_numpy() mjd_utc = times.to_numpy(scale="utc") mjd_utc = times.to_mjd(scale="utc") # the symmetry between .from_mjd() and .to_mjd() feels quite intuitive

spenczar · 2023-10-02T23:12:43Z

adam_core/coordinates/covariances.py

@@ -153,6 +153,25 @@ def is_all_nan(self) -> bool:
        """
        return np.all(np.isnan(self.to_matrix()))

+    @classmethod
+    def nulls(cls, length: int) -> "CoordinateCovariances":


This function is helpful when creating a "null" covariances which can be written to and read from Parquet, because apache/arrow#35692 remains an issue.

I wrote something similar and it's been super useful. This is great!

spenczar · 2023-10-02T23:15:56Z

adam_core/dynamics/propagation.py

+            time=Timestamp.from_astropy(
                Time(t1_, scale="tdb", format="mjd"),
            ),


There are a lot of Timestamp.from_astropy(astropy.Time(...)) calls littered throughout the codebase. Those should be shrinkable down to just Timestamp(...) in some way - possibly a new direct constructor. For example here it would be `Timestamp.from_mjd(t1_, scale="tdb").

I could do those now, if that seems useful.

If you are feeling inspired, then yes absolutely! All the .from_astropy(Time()) should go since it's pretty silly. I like .from_mjd(scale=...), it's explicit and very convenient.

Yes, definitely.

spenczar · 2023-10-02T23:17:14Z

adam_core/observers/tests/test_state.py

+@pytest.mark.parametrize("code", ["X05", "I41", "W84"])
+@pytest.mark.parametrize("origin", ["sun", "ssb"])
+def test_get_observer_state(code, origin):


This file looks like it got a lot of changes, but really, this is just parametrization to reduce the repetition. It doesn't do anything novel.

It's much better this way.

spenczar · 2023-10-02T23:17:43Z

adam_core/observers/tests/testdata/get_states.py

@@ -0,0 +1,48 @@
+import numpy as np


This file's contents come from the README, more or less.

spenczar · 2023-10-02T23:18:30Z

adam_core/orbits/variants.py

-                "jd1": orbits.coordinates.time.jd1,
-                "jd2": orbits.coordinates.time.jd2,
+                "day": orbits.coordinates.time.days,
+                "millis": orbits.coordinates.time.millis(),


This is a consequential choice - what's the linking precision for these variants? I figured milliseconds is good enough, but maybe micros? Maybe seconds? Maybe nanos? You tell me.

I think we might want to define the link precision as a kwarg in this function? When variants are created they will inherit the time of their parent in which case nanos (or whatever the smallest is) would be appropriate. Once we do something to those variants, say we propagate them with PYOORB to some time. There is no guarantee that PYOORB doesn't return a slightly different time for each variant (especially if they might be propagated in different processes where the stepsize might change for that particular chunk of orbits). In this case, I don't know really what to expect.

Maybe let's set it to nanos and be aggressive? If we bump into linkages that have 0 members in one of the tables then we revisit the constraints?

On the plus side, this is the beauty of this Timestamp class. We can define those link precisions and that is super nice. I think its definitely a point worth highlighting when we demo the code to others.

moeyensj

Early review comments and please push back where necessary.

One thing to check so we avoid continually reverting/redoing changes to the test data is if your astropy cache is affecting the test data diffs.

The only other "major" thing is the interface of the Timestamp class. This will likely replace our use of astropy time so it would be good I think to come up with an API we are happy with. I think astronomers will expect to see something astropy-esque. Having that level of convenience and ease of use would be nice. We may want to consult @mjuric, or better maybe we get you to do a quick show and tell in the DiRAC Solar System meeting to get opinions there.

Lastly, I think we should go "all-in" and change the interfaces of our primary functions (the ones in test_imports.py that are not jax-jitted like propagator.propagate_orbits) to accept Timestamp objects and not astropy Time objects. This could be done in a separate PR if needed but it would inform how much we like the interface to the Timestamp class.

moeyensj · 2023-10-02T23:21:37Z

adam_core/coordinates/cometary.py

-            cartesian.time.to_astropy().tdb.mjd,
+            cartesian.time.to_numpy(),


I think to_numpy is a good addition. Though I agree that the name is a little opaque. Do you think it might make sense to add a scale kwarg that will return the numpy array in the desired scale (to_numpy(scale="tdb"))? The default can be set to its current scale?

moeyensj · 2023-10-02T23:22:50Z

adam_core/coordinates/covariances.py

@@ -153,6 +153,25 @@ def is_all_nan(self) -> bool:
        """
        return np.all(np.isnan(self.to_matrix()))

+    @classmethod
+    def nulls(cls, length: int) -> "CoordinateCovariances":


I wrote something similar and it's been super useful. This is great!

moeyensj · 2023-10-02T23:28:08Z

adam_core/dynamics/propagation.py

+            time=Timestamp.from_astropy(
                Time(t1_, scale="tdb", format="mjd"),
            ),


If you are feeling inspired, then yes absolutely! All the .from_astropy(Time()) should go since it's pretty silly. I like .from_mjd(scale=...), it's explicit and very convenient.

moeyensj · 2023-10-02T23:29:19Z

adam_core/observers/tests/test_state.py

+@pytest.mark.parametrize("code", ["X05", "I41", "W84"])
+@pytest.mark.parametrize("origin", ["sun", "ssb"])
+def test_get_observer_state(code, origin):


It's much better this way.

adam_core/utils/helpers/data/elements_ssb_ec.csv

moeyensj · 2023-10-02T23:50:55Z

adam_core/orbits/variants.py

-                "jd1": orbits.coordinates.time.jd1,
-                "jd2": orbits.coordinates.time.jd2,
+                "day": orbits.coordinates.time.days,
+                "millis": orbits.coordinates.time.millis(),


I think we might want to define the link precision as a kwarg in this function? When variants are created they will inherit the time of their parent in which case nanos (or whatever the smallest is) would be appropriate. Once we do something to those variants, say we propagate them with PYOORB to some time. There is no guarantee that PYOORB doesn't return a slightly different time for each variant (especially if they might be propagated in different processes where the stepsize might change for that particular chunk of orbits). In this case, I don't know really what to expect.

Maybe let's set it to nanos and be aggressive? If we bump into linkages that have 0 members in one of the tables then we revisit the constraints?

moeyensj · 2023-10-02T23:52:45Z

adam_core/orbits/variants.py

-                "jd1": orbits.coordinates.time.jd1,
-                "jd2": orbits.coordinates.time.jd2,
+                "day": orbits.coordinates.time.days,
+                "millis": orbits.coordinates.time.millis(),


On the plus side, this is the beauty of this Timestamp class. We can define those link precisions and that is super nice. I think its definitely a point worth highlighting when we demo the code to others.

adam_core/time/interval.py

moeyensj · 2023-10-03T00:03:57Z

adam_core/coordinates/cometary.py

-            cartesian.time.to_astropy().tdb.mjd,
+            cartesian.time.to_numpy(),


Another option (now having read more of the code) might be to enable symmetry between .from_mjd by adding a to_mjd(scale="tdb"). The current mjd() could suffice as well with a scale parameter. The latter could just return a pyarrow array (the additional call to .to_numpy() from there doesn't seem super cumbersome from my perspective).

I suspect a lot of astronomers will expect an interface similar to astropy. Don't know how much we want to mimic it but that's a thought. This doesn't have to be addressed in this PR obviously. Here are some options:

import numpy as np from adam_core.time import Timestamp mjd_tdb = np.arange(59000, 60000) times = Timestamp.from_mjd(mjd_tdb, scale="tdb") # this feels very nice and not much different than astropy mjd_utc = times.utc().mjd().to_numpy() mjd_utc = times.mjd(scale="utc").to_numpy() mjd_utc = times.to_numpy(scale="utc") mjd_utc = times.to_mjd(scale="utc") # the symmetry between .from_mjd() and .to_mjd() feels quite intuitive

moeyensj · 2023-10-03T00:19:32Z

I forgot to say this in the review: These are excellent changes and I'm really excited to be able to move away from insane floating point time madness.

akoumjian · 2023-10-03T19:16:48Z

adam_core/observations/exposures.py

@@ -19,7 +18,7 @@ class Exposures(qv.Table):
    """

    id = qv.StringColumn()
-    start_time = Times.as_column()
+    start_time = Timestamp.as_column()
    duration = qv.Float64Column(validator=and_(ge(0), le(3600)))


I wonder if switching this to an integer (nanoseconds?) would be worth the inconvenience?

Convenience method for converting to MJD TDB as an array of numpy float64s

Use parquet instead of CSV for data files because it can stored fixed-length lists. Add utilties for creating CoordinateCovariances which are full of nulls, since that's necessary when reading fixed length lists with null data out of parquet. Condense and parametrize the test case for observer state computation. Obey lint

spenczar requested a review from moeyensj October 2, 2023 23:10

spenczar commented Oct 2, 2023

View reviewed changes

moeyensj reviewed Oct 3, 2023

View reviewed changes

akoumjian reviewed Oct 3, 2023

View reviewed changes

spenczar force-pushed the use_integer_time branch from 6f8b3f6 to de372bc Compare October 5, 2023 00:42

spenczar added 14 commits October 10, 2023 12:15

Add utilities for converting to TDB and from MJD

be47a92

Add Timestamp.to_numpy

940b220

Convenience method for converting to MJD TDB as an array of numpy float64s

Use integer Timestamps throughout codebase

d42b79a

Re-delete coordinates/io.py

90007a8

Add Timestamp.unique

a964474

Use integer time throughout SPICE and observer state code

c9c9f79

Replace trivial uses of astropy with direct construction of Timestamps

19ac3dd

Update benchmarks to use integer time

e5dcc63

Add Timestamp.rescale

a82e444

Use integer time in all interfaces, including propagators

0bf9f32

Remove straggling places where astropy time is used

24a8430

Fix time import

dda3e18

Delete empty file

382301c

spenczar force-pushed the use_integer_time branch from ca3b634 to 382301c Compare October 10, 2023 20:02

Fix up mishmash that was missed in rebase

7b5a46f

spenczar merged commit 461087a into main Oct 10, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use integer time throughout codebase #73

Use integer time throughout codebase #73

spenczar commented Oct 2, 2023

spenczar Oct 2, 2023

moeyensj Oct 2, 2023

moeyensj Oct 3, 2023

spenczar Oct 2, 2023

moeyensj Oct 2, 2023

spenczar Oct 2, 2023

moeyensj Oct 2, 2023

akoumjian Oct 3, 2023

spenczar Oct 2, 2023

moeyensj Oct 2, 2023

spenczar Oct 2, 2023

spenczar Oct 2, 2023

moeyensj Oct 2, 2023

moeyensj Oct 2, 2023

moeyensj left a comment

moeyensj Oct 2, 2023

moeyensj Oct 2, 2023

moeyensj Oct 2, 2023

moeyensj Oct 2, 2023

moeyensj Oct 2, 2023

moeyensj Oct 2, 2023

moeyensj Oct 3, 2023

moeyensj commented Oct 3, 2023

akoumjian Oct 3, 2023

		cartesian.time.to_astropy().tdb.mjd,
		cartesian.time.to_numpy(),

Use integer time throughout codebase #73

Use integer time throughout codebase #73

Conversation

spenczar commented Oct 2, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moeyensj 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

moeyensj commented Oct 3, 2023

Choose a reason for hiding this comment