fix: parsing of jyear and byear times from ASDF

[embray]

The schema allows value: JXXXX.X with format: jyear and likewise for Bessellian years. Astropy is a bit overly strict in requiring the 'jyear_str' and 'byear_str' Time formats for these when instantiating a Time with a known format.

After experimenting with different approaches it seems, ufortunately, most reliable to refuse to guess, and instead take a test value (if the node value is an array or quantity) and check if it is a string starting with the appropriate prefix.

This came up in asdf-format/libasdf#201 , as libasdf allows writing a time like:

obstime: !time/time-1.4.0
  value: J1948.78707178
  format: jyear

which is valid within the schema.

Arguably Astropy's behavior here is finicky and misleading:

>>> Time('J1948.78707178', format='jyear')
Traceback (most recent call last):
<... long traceback ...>
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
  Cell In[37], line 1
    t = Time('J1948.78707178', format='jyear')
  File ~/src/asdf-format/asdf-astropy/.venv/lib/python3.12/site-packages/astropy/time/core.py:1991 in __init__
    self._init_from_vals(
  File ~/src/asdf-format/asdf-astropy/.venv/lib/python3.12/site-packages/astropy/time/core.py:559 in _init_from_vals
    self._time = self._get_time_fmt(
  File ~/src/asdf-format/asdf-astropy/.venv/lib/python3.12/site-packages/astropy/time/core.py:643 in _get_time_fmt
    raise ValueError(
ValueError: Input values did not match the format class jyear:
TypeError: for jyear class, input should be (long) doubles, string, or Decimal, and second values are only allowed for (long) doubles.

The error message is misleading because it says the input may be a "string", which it was. But what it's looking for here is a bare decimal string like 2000.0.

I can see why, for serialization purposes, Astropy has separate 'jyear' and 'jyear_str' formats (though arguably the latter should be a subformat of the former). But I think it could stand to be a little more flexible on this too when there's no ambiguity.

Also fixed a bit of what appeared to be copy/paste detritus in one of the other existing test cases.

[codecov]

Codecov Report

❌ Patch coverage is 72.72727% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 98.84%. Comparing base (f812d9e) to head (a5dbfd0).
⚠️ Report is 27 commits behind head on main.

Files with missing lines	Patch %	Lines
asdf_astropy/converters/time/time.py	72.72%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #326      +/-   ##
==========================================
- Coverage   99.06%   98.84%   -0.23%     
==========================================
  Files          71       71              
  Lines        2572     2594      +22     
==========================================
+ Hits         2548     2564      +16     
- Misses         24       30       +6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[braingram]

I'm not sure we should consider

obstime: !time/time-1.4.0
  value: J1948.78707178
  format: jyear

as something we want to support since t's inconsistent with respect to the typical time formatting (where J* is jyear_str not jyear). The implementation in the current PR doesn't round-trip where reading in above produces a time with format jyear_str which is then written out as:

obstime: !time/time-1.4.0 {scale: tt, value: J1948.787}

which is no longer jyear.

[embray]

I don't think jyear_str should even exist as a separate format, tbh. I'd be wary of making format design decisions that are very specific to questionable implementation artifacts in Astropy. The JXXXX format is purely representational with the unit specified to disambiguate a raw numerical value where no format is otherwise specified.

It's a good point about the round-tripping question though. But I think that could also be fixed in coordination with Astropy. Let me think about it...

[embray]

In either case the schema is not strictly correct, or rather, overly permissive. It just has:

    properties:
      value:
        description: |
          The value(s) of the time.

        anyOf:
          - $ref: "#/definitions/string_formats"
          - $ref: "#/definitions/array_of_strings"
          - $ref: "../core/ndarray-1.1.0"
          - type: number

      format:
        description: |
          The format used to save the time in ASDF

          If not provided, the the format should be guessed from the
          string from among the following unambiguous options:
          `iso`, `byear`, `jyear` and `yday`.

        $ref: "#/definitions/format"

which actually allows complete mismatches between format and value. Should be, schematically, ignoring the array case for now:

oneOf:
  - value: {$ref: "#/definitions/iso_time"}
    format: iso
  - value: {$ref: "#/definitions/byear"}
    format: byear

and so on.

The other_format definitions are also completely unspecified if/how they should be used.

I also noticed while hacking on the time parser in libasdf that some of these regular expressions seem to be trying to encode too much information, like:

yday: "[0-9]{4}:(00[1-9])|(0[1-9][0-9])|([1-2][0-9][0-9])|(3[0-5][0-9])|(36[0-5]):([0-1][0-9])|([0-1][0-9])|(2[0-4]):[0-5][0-9]:[0-5][0-9](.[0-9]+)?"

I simplified it to just: "^(\\d\\d\\d\\d):(\\d\\d\\d):(\\d\\d):(\\d\\d):(\\d\\d)(.\\d+)?". The current one is trying to encode every possible valid date, but it can't handle leap years (impossible in a regular expression), nor does it capture days of months correctly.

[braingram]

I don't think jyear_str should even exist as a separate format, tbh. I'd be wary of making format design decisions that are very specific to questionable implementation artifacts in Astropy. The JXXXX format is purely representational with the unit specified to disambiguate a raw numerical value where no format is otherwise specified.

The comment about jyear_str reflects the current state of the time tag/schema:
https://github.com/asdf-format/asdf-standard/blob/786797692c6185cfbacfaf0e1de3b55fb2f33e9e/resources/stable/schemas/stsci.edu/asdf/time/time-1.4.0.yaml#L203
Untangling it from astropy would be a separate issue.

For the other issues about the time schema would you open an asdf-standard issue?

What's preventing updating libasdf to write out times matching what asdf-astropy currently does?

[embray]

See discussion in astropy/astropy-APEs#134 ; clearly need to think more about what to do here. I still posit that jyear_str and byear_str should not be distinct formats in the ASDF schema at all, and to avoid inheriting specific design decisions from Astropy. If/how this will be changed on the Astropy side is an open question, but this can also be smoothed over pretty easily by the Converter in this package, I think.