1292 bug epoch time should be tdt instead of tdb

[subagonsouth]

Change Summary

Overview

Fix issue with spice time functions reporting TDB time instead of TT for CDF epoch values. This PR had to touch lots of files due to the function renaming. To make review easier, I suggest that you review in two segments. In the File Changes tab, select the first commit to view just the changes that were made to the imap_processing.spice.time.py module and associated tests. Once finished with that, view the other commit that pushed the function and global variable name changes in that module to all of the locations that they were used.

Updated Files

I'm not going to list all of the files to save time. The important changes are detailed below.

• imap_processing/spice/time.py
  • Renamed global variable in spice.time from J2000_EPOCH to J2000_TT_EPOCH and updated value to TT timescale.
  • Renamed met_to_j2000ns function to met_to_tt_ns and modified it to return TT nanoseconds rather than TDB nanoseconds.
  • Renamed j2000ns_to_j2000s function to tt_ns_to_et and added conversion from TT to ET.
  • Added sct_to_tt function to aid conversion from spacecraft clock ticks directly to TT with one for loop instead of chaining together multiple looping functions.
• imap_processing/tests/spice/test_time.py
  • Updated tests to correctly validate above changes.
  • Added functional test coverage for the new sct_to_tt function.

Closes: #1292

[maxinelasp]

Thanks for the quick update! Overall looks great, I had a few minor suggestions that you can take or leave.

[maxinelasp]

Nice explanation!

[maxinelasp]

What if we move these to point to the J2000_TT_EPOCH constant variable?

[subagonsouth]

Not sure I understand. I don't know exactly what these commented out lines are for in this test, but they are pointing to the J2000_TT_EPOCH constant variable.

[greglucas]

It seems like we have several functions with slightly different names all doing similar conversions. Would it be beneficial to combine these into a single function with keyword argument inputs so that we are just referencing this one function with different parameters as we need them? Just a thought I'm writing down in case this appeals to you, no need to do it here.

def convert_time(my_time_in_whatever_frame, input_frame="TT", output_frame="ET"):
    # Some logic to select your input functions
    ....
    # Vectorize the spiceypy calls
    np.vectorize(func, ...)

[greglucas]

Is it worth inverting the np.vectorize you have in the function above (that one goes from TT to ET)? This is a for loop within Python currently.

[subagonsouth]

I'm happy to do that although my understanding of np.vectorize is that it is a python loop anyway. The main thing that I was trying to do here was avoid two for loops by doing these two steps of converting from s/c ticks to ET and then ET to TT each in their own loop. I could use np.vectorize on a functools.partial of the two chained spiceypy functions.

Side question: Do you think all of the manually "vectorized" functions in this module would benefit from using np.vectorize? My only complaint is that np.vecotrize returns an array with shape=() when a scalar is input.

[greglucas]

You are right, the documentation even says there is basically no speedup.
https://numpy.org/doc/stable/reference/generated/numpy.vectorize.html

The vectorize function is provided primarily for convenience, not for performance. The implementation is essentially a for loop.

So I guess the answer is maybe it isn't worth it and just write the np.array([value for value in list]) loop like you already are. I think the reason to use it is for the broadcasting types then? So if you wanted your function to accept a 0d, 1d, 2d arrays without having to do the for loops yourself for those cases.

[subagonsouth]

It seems like we have several functions with slightly different names all doing similar conversions. Would it be beneficial to combine these into a single function with keyword argument inputs so that we are just referencing this one function with different parameters as we need them? Just a thought I'm writing down in case this appeals to you, no need to do it here.

@greglucas I agree. I'll write a new ticket to explore the idea of one time conversion function to rule them all. Brandon has done something quite complicated in the curreyer library to accomplish that goal. I am undecided if I like it or not.