Compatibility with ASDF v3

[pllim]

Fix #1011 and fix #664

cc @braingram

TODO

• Relax jsonschema pinning
• Fix test failures

🐶 🐱

[WilliamJamieson]

Some thoughts to maybe get this working.

[pllim]

@WilliamJamieson , can you please explain why there is a need to change $ref to tag here but not in spectrum_list-1.0.0.yaml? Thanks!

[WilliamJamieson]

There is no change from a $ref to a tag here.

The $ref that I removed was a local reference which pointed at the schema that was removed as part of this PR. Meaning this schema was broken by that removal. However, the removed schema was nearly identical to a schema in asdf-coordinates-schemas (I'll address the difference below). In fact, it duplicated the URI, which means one of the two versions would override the other. Which in general can cause confusion (no schemas should have the same URI). Indeed, looking at how ASDF loaded the schemas the asdf-coordinates-schema would always be discovered first meaning the procided here would never be used (except in the test generated by the pytest-asdf plug-in). The one difference was that it included a tag within the schema, which I will discuss why this is unnecessary below.

Instead, what is going on here is I generated a new tag for an existing schema (multiple tags can point to the same schema), using the manifest. Moreover, the schema that tag points to is the one from asdf-coordinates schemas discussed above.

In point of fact, this tag is "redundant" because the difference between a tag and a $ref, assuming the tag points to the same schema as the $ref, is that in addition addition to validating the data against the schema, the tag is on that sub tree is also checked. Note that the tag is also checked if the schema lists a tag; however, doing so is strongly discouraged because it prevents reuse of the schema by new tags (for new objects which have the same representation but different behaviors like the difference between SpectralAxis and SpectralCoord). That is it prevents the ability to have multiple tags pointing at the same schema, which is exactly what I did here to avoid unnecessary duplications.

To explicitly see the redundancy, observe the anyOf statement being applied to the list of statements This means any of the listed things is acceptable. Since the spectral_axis points to the schema which is referenced by the $ref on the next line. This means that spectral_axis tags would validate against the $ref anyways. As any tagged object (regardless of the tag itself) which satisfies the $ref will pass that line. Thus the tag added is purely of documentation value (this spectral axis tag is one possibility). Really, the spectralcood tag should be added for completeness.

Note that in general, it is better to use a tag than a $ref in this situation because it makes explicit the object type(s) one expects, unless you are working with something which can accept a broad category of similar objects (in terms of how you actually write them down).

This means I would prefer to remove the $ref all together in this schema, but as this asdf extension was poorly maintained I fear existing files may fail to validate under these tighter restrictions. Meaning I did not do that (for now). Similarly, the spectral_list really ought to be of the tag for this schema not the $ref, but again the same caution is advised.

I suggest that once this PR is merged, a second PR be opened. In this new PR new versions of the existing schemas be introduced (alongside a new manifest). These schemas would have the $ref to tag changes I discussed above. This would allow for reading/validating existing files via the old manifest, but strictly enforce things on any new files as ASDF will default to writing under the latest available tags/schema.

[pllim]

Thank you very much for the very detailed explanation!

[pllim]

• dev job is pulling in older scipy because of gwcs (see Remove maxversion pin of scipy spacetelescope/gwcs#447), nothing we can do here.
• jsonschema pin is due to jsonschema 4.18 incompatible with asdf asdf-format/asdf#1485

[pllim]

Note to maintainer: This no longer xfail

[codecov]

Codecov Report

Attention: Patch coverage is 99.14530% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.14%. Comparing base (8179313) to head (aa18380).
Report is 89 commits behind head on main.

Files with missing lines	Patch %	Lines
specutils/io/asdf/extension.py	92.85%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1042      +/-   ##
==========================================
+ Coverage   70.01%   70.14%   +0.12%     
==========================================
  Files          64       64              
  Lines        4346     4401      +55     
==========================================
+ Hits         3043     3087      +44     
- Misses       1303     1314      +11     

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

[pllim]

William has been an invaluable help to get things to really work here. Thanks, William!

[rosteen]

Thanks for this @pllim and @WilliamJamieson, sorry for taking so long to get around to reviewing it. I was able to install and run the tests locally with dev asdf, so that's a good sign. The only concern I have on my initial read-through of the code is with two things in Spectrum1DConverter:

1. We should round-trip the mask attribute as well as the uncertainty.
2. I think it would be better to round-trip the wcs rather than the spectral_axis, so that any spatial information in a multi-dimensional Spectrum1D also gets preserved (and the spectral_axis will be reconstructed from the wcs). I assume that asdf can encode both FITS WCS and GWCS, but I don't actually know that for sure off the top of my head/remember which direction dependencies go - is asdf only applicable in the GWCS case anyway? We currently aren't actually preserving spatial information from JWST cubes right now anyway, but it's at the top of my list specutils of to-dos in the next couple weeks.

The SpectrumListConverter looks like magic to me, but judging by the tests it works 😆.

[pllim]

The roundtrip comments -- Was it happening with old version of asdf but not anymore, or is this more like a new feature request? I don't recall that this PR would remove any existing features.

[rosteen]

The roundtrip comments -- Was it happening with old version of asdf but not anymore, or is this more like a new feature request? I don't recall that this PR would remove any existing features.

I wasn't familiar with the old asdf code here, but it looks like these were also missing from the old code based on the diff. Still, this is a good chance to fix either of those if they are simple to add, but it could also be a follow-up PR if you don't want to implement here.

[pllim]

It would not be simple for me, as I am unfamiliar with ASDF and cannot get to it in the next 2 weeks. I would prefer it to be a new ticket, especially since William has already rotated off the ASDF work and might not have time to jump in.

[rosteen]

Alright, I opened a followup issue for that. I'll approve and merge this.

[pllim]

Thank you!