Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Time values do not always round trip perfectly #133

Open
WilliamJamieson opened this issue Nov 22, 2022 · 6 comments
Open

Time values do not always round trip perfectly #133

WilliamJamieson opened this issue Nov 22, 2022 · 6 comments

Comments

@WilliamJamieson
Copy link
Contributor

Due to how we are generating the representation for astropy.time.Time objects, some numerical inaccuracies accumulate. This results in the time value read from asdf for a astropy.time.Time object slightly differing the value of the original object. The reason for this is that several of the available time formats require a conversion into a format which we write to ASDF (for human readability) and then that format will get converted to the original format, which is a process that generates numerical errors.

A demonstration of this is the need for a atol and the use of isclose instead of == in this test:
https://github.com/WilliamJamieson/asdf-astropy/blob/889005badaea78a6feb59f72574d3b9b299292b9/asdf_astropy/converters/time/tests/test_time.py#L139-L153 (fixing an incorrectly written test as part of #132).

Currently, there are two possible solutions that I see:

  1. astropy.time.Time should be written under a new schema (say http://astropy.org/schemas/astropy/time/time-1.0.0) rather than the one built into ASDF. This schema should instead write the base-representation (using val1 and val2 directly) for the astropy.time.Time object rather than attempting to write the object in a human-readable format. @perrygreenfield suggested that maybe ASDF should support converters including a comment alongside this to make it human readable.
  2. As proposed by @eslavich, use the decimal built-in type of python to exactly convert val1 and val2 into their exact representation for the universal time value, and write that value as an exact string. Then forcing the read in decimal followed by a computation in decimal to extract val1 and val2. Maybe include a flag in the existing time schema to indicate this mode should be used.

Other solutions may exist, which should be discussed as part of this issue.

@WilliamJamieson
Copy link
Contributor Author

Tagging @mhvk and @taldcroft as the experts on astropy.time for comment if they are interested.

@taldcroft
Copy link
Member

I think that if you write the jd1 and jd2 values using the Python repr that is an exact representation of the value (to 64 bits) and should round trip exactly. There is precedent (and example code) in the ECSV handling which allows representing a Time object using jd1_jd2 format (e.g. https://docs.astropy.org/en/stable/io/unified.html#table-serialization-methods).

@mhvk
Copy link

mhvk commented Nov 22, 2022

I think option (1) seems better as it is guaranteed to work even if new time formats are introduced, while for option (2) you'll need to special case almost every format. The suggestion of a comment with the normal representation makes sense to me. As @taldcroft notes, we had the same problem for writing times to ascii tables, and ended up giving the user the choice.

@WilliamJamieson
Copy link
Contributor Author

WilliamJamieson commented Nov 22, 2022

Besides storing jd1 and jd2 and the current format being used by the Time object, what other things must be stored to exactly reproduce a Time object?

Looks like we need location, scale, precision, in_subfmt, and out_subfmt, in addition to val1, val2, and format.

@taldcroft
Copy link
Member

See https://github.com/astropy/astropy/blob/cffe98aaff7a5ffe442934ecdf20b209606db0db/astropy/time/core.py#L182

In particular the _represent_as_dict_attrs property. For a Time object you can do tm.info._represent_as_dict() to see this in action.

@mhvk
Copy link

mhvk commented Nov 22, 2022

Indeed, _represent_as_dict() should be useful for many of astropy's classes - at least those that can be stored in tables!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants