Skip to content

Conversation

@braingram
Copy link
Contributor

@braingram braingram commented Aug 27, 2025

The reorganization in #480 broke a few time formats since I missed that in the time schema in addition to the oneOf in base_format:

oneOf:
- $ref: "#/definitions/format"
- $ref: "#/definitions/other_format"

there is an exclusive consideration of only the formats listed in format in format:

This fixes the broken formats (cxcsec, galexsec, unix_tai) by moving them from other_format to format.

This PR is tested in astropy/asdf-astropy#270 (by setting the asdf-standard pin in that PR to this PR). I will open an issue to discuss moving these "astronomy" schemas to asdf-astropy since this PR highlights the problematic relationship of these two packages (asdf-standard has the astronmy schemas but can't test them without asdf-astropy which needs the schemas to implement support). The "asdf-standard" downstream failure in that PR is another sign of the mess that is these two packages (the job is testing asdf-standard main after install the source branch for this PR).

@perrygreenfield
Copy link
Contributor

In looking at the previous PR I was under the impression that the other_formats were never saved as such, but were indicators that the value stored was derived from them and that the base_format only indicates what it was derived from. But this isn't the case? How does base_format relate to the format?

@braingram
Copy link
Contributor Author

braingram commented Aug 27, 2025

In looking at the previous PR I was under the impression that the other_formats were never saved as such, but were indicators that the value stored was derived from them and that the base_format only indicates what it was derived from. But this isn't the case? How does base_format relate to the format?

It looks like base_format is always the Time.format:
https://github.com/astropy/asdf-astropy/blob/a53570f6fcb1312445a3986696e635d7e94096fe/asdf_astropy/converters/time/time.py#L21
(although it is only written if base_format != format)
(and format is only written if it's not "guessable").
So for isot the output is:

time: !time/time-1.4.0 2025-08-27T15:38:25.758

(no format, no base_format as this is guessable)
for byear the output is:

time: !time/time-1.4.0 {base_format: byear, value: B2025.656}

(i think because "B2025.656" is "guessable" as byear_str but the provided/round-tripped format is byear)
for byear_str the output is:

time: !time/time-1.4.0 B2025.656

for mjd the output is:

time: !time/time-1.4.0 {format: mjd, value: 60914.65168701704}

(format since this is not "guessable").
Perhaps this can be simplified to:

  • always write the format
  • list all formats supported by astropy

It also looks like there a bugs (not introduced by this or the previous PR) since for byear above the provided time does not round trip:

>> t
<Time object: scale='utc' format='byear' value=2025.65590263171>
>> asdf.loads(asdf.dumps({"time": t}))['time']
<Time object: scale='tt' format='byear' value=2025.656>

Copy link
Contributor

@perrygreenfield perrygreenfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@braingram braingram merged commit 6d2a44e into asdf-format:main Aug 27, 2025
16 checks passed
@braingram braingram deleted the fix_time branch August 27, 2025 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants