bradsease / oem

Python tools for working with Orbit Ephemeris Messages (OEMs)
MIT License
14 stars 5 forks source link

KVN file created using `save_as` is not loadable #83

Closed kyle-cochran closed 4 months ago

kyle-cochran commented 4 months ago

Describe the bug Hey, great library! I am hoping to use this as part of the framework for a library I'm working on to easily interact with the various SSA providers. So far it's a great experience. Very clean and readable. Kudos!

I noticed while testing with it that, while I am able to load a KVN file in general, I am not able to load one of the KVN files that is produced from the OrbitEphemerisObject itself. It fails with the error: ValueError: States in data section are not ordered by epoch Example below. The initial input OEM file is attached. Let me know if I'm just doing something wrong and this is not a real issue. Thanks!

Steps to Reproduce

from oem import OrbitEphemerisMessage

ephemeris = OrbitEphemerisMessage.open("oem_file_ccsds.txt") # works
ephemeris.save_as("oem_output.kvn") # by visual inspection, looks like a correct OEM
ephemeris = OrbitEphemerisMessage.open("oem_output.kvn") # fails

oem_file_ccsds.txt

Python/Package Version Information Python: [3.11.3] oem: [0.4.1]

kyle-cochran commented 4 months ago

With some experimentation, I have found a probable cause. The input file oem_file_ccsds.txt contains two points at the end that are very close in time: 2024-02-08T19:46:03.597928Z 2024-02-08T19:46:03.597932Z

They are 6 µs apart. Python supports up to microsecond precision, so this is fine for the first input file.

However when the new file is saved, the timestamps are truncated to millisecond precision: 2024-02-08T19:46:03.598 2024-02-08T19:46:03.598

This throws an error, since there are repeating timestamps.

kyle-cochran commented 4 months ago

@bradsease I am happy to open a merge request with a fix for this, if you wouldn't mind providing some input on the preferred solution from your end. I can see a few possible approaches:

1.) do not truncate timestamps when outputting files 2.) allow duplicate timestamps [probably could lead to some issues] ⚠️ 3.) automatically filter duplicate timestamps 4.) only allow millisecond input precision 5.) simply improve the error message for duplicate input points

Let me know your preference on approach, or if you have something else in mind entirely, and I can open an MR.

bradsease commented 4 months ago

Good find, thanks for digging into it!

I definitely lean toward # 1 and I feel like it is a bug causing the timestamps to be shortened. I'm not sure why that's happening, because the format_epoch function calls strftime under the hood and that should be returning microsecond precision..

bradsease commented 4 months ago

A quick test shows that astropy.Time is not handling strftime the way I would expect, so I think that is the source of the issue. Converting it to a datetime first seems to resolve it, but that might cause some other issues

>>> Time('2024-02-08T19:46:03.597928Z', format='isot', scale='utc').strftime("%Y-%m-%dT%H:%M:%S.%f")
'2024-02-08T19:46:03.598'
>>> Time('2024-02-08T19:46:03.597928Z', format='isot', scale='utc').datetime.strftime("%Y-%m-%dT%H:%M:%S.%f")
'2024-02-08T19:46:03.597928'
kyle-cochran commented 4 months ago

@bradsease thanks, that's a helpful find!

Here's an MR with a fix: https://github.com/bradsease/oem/pull/84

Feel free to edit, etc as you see fit

bradsease commented 4 months ago

Thanks again for looking into this! Just pushed version 0.4.2 that includes the fix