Asdf-Support

[ViciousEagle03]

This PR adds ASDF support for ndcube objects.

GitHub Project Board Link

This Pull Request is currently a Work In Progress and will be continuously updated as we expand support for serializing ndcube objects to ASDF.

[braingram]

Thanks for sharing your work. It looks like a great start! I left a bunch of comments. I did not look over the ndcube.py changes.

[braingram]

I would leave this out for now. As you've noted in the comments (which are very helpful!) the ordering here can sometimes lead to confusion. The tag versions can be managed by versioning the extension (in different manifests) so that for a given extension there is only one tag version (and we won't need a select_tag).

[braingram]

Would you also enable schema tests for the new schemas? Here's the configuration for asdf-astropy:
https://github.com/astropy/asdf-astropy/blob/1b286108042c1201199e9931cd2a5b52cc57bdce/pyproject.toml#L121-L122

I am not familiar with the ndcube tox and github actions CI but would you look into modifying whatever is necessary there to get the schema tests running for the PR? Let me know if there are more details or anything I can do to help with this. Also, perhaps one of the other maintainers has a sense for how these schema tests might be integrated in the CI.

[ViciousEagle03]

I have enabled the schema test, @Cadair could you please verify if the configuration is correct?
I followed this to enable the tests. Thanks!

[braingram]

I don't think this has allowed the schema tests to run in the CI. I think it's related to asdf-format/asdf#1756

The core issue is that the CI run is testing files in the installed package but is running from a temporary directory. Take this test as an example:

../../.tox/py312/lib/python3.12/site-packages/ndcube/extra_coords/tests/test_extra_coords.py::test_empty_ec PASSED

The path to the file contains a bunch of stuff that is very specific to that test run (py312, tox, etc) and this causes the asdf pytest plugin to ignore the schema files since they appear at a similar path which doesn't match the asdf_schema_root.

As there are test configurations where we might expect the schema tests to fail (like the oldest deps job, see this conversation for more details) we may want to consider adding a new CI job (or jobs) that test just the schemas. Minimally they could be tested against the newest released asdf and perhaps they could also be tested against development versions of asdf and the dependent schema packages. Ignoring the development testing at the moment I think this would involve:

• add a tox factor that runs just the schema tests (defining asdf_schema_root, providing a path to the schemas, not running in a temporary directory)
• enabling this factor in the CI workflow

[ViciousEagle03]

I've included a separate environment for schema testing in tox and created a workflow for it. Should I add a separate job for the schema tests as well? Please let me know. (P.S. I got a bit carried away with the workflow concept.)
The schema tests(now enabled) in the CI now doesn't run from a .tmp directory:

../../ndcube/asdf/resources/manifests/ndcube-0.1.0.yaml .                [ 16%]
110
../../ndcube/asdf/resources/schemas/ExtraCoords-0.1.0.yaml .             [ 33%]
111
../../ndcube/asdf/resources/schemas/NDCube-0.1.0.yaml .                  [ 50%]
112
../../ndcube/asdf/resources/schemas/QuantityTableCoordinate-0.1.0.yaml . [ 66%]
113
                                                                         [ 66%]
114
../../ndcube/asdf/resources/schemas/SkyCoordTableCoordinate-0.1.0.yaml . [ 83%]
115
                                                                         [ 83%]
116
../../ndcube/asdf/resources/schemas/TimeTableCoordinate-0.1.0.yaml .     [100%]

[braingram]

The workflow and tox changes you've included so far look good enough to me. I don't think adding a separate job is necessary (but please let me know if I'm missing something that would provide). Thanks!

[Cadair]

This is coming on great @ViciousEagle03 thanks so much for all your work.

[Cadair]

If I subclass NDCube and want to write my own schema for my subclass with additional properties does this get in the way? @braingram

[braingram]

I think so. A typical way to "subclass" a schema is to $ref the parent schema in a allOf. Using allowAdditionalProperties: False here would prevent the sub-schema from adding properties (because any addition would cause the parent schema validation to fail, causing the allOf to fail).

[braingram]

@ViciousEagle03 Would you add some tests to this PR that verify that a variety of NDCube objects can be written to and read from an asdf file? For some inspiration, here are some examples in asdf-astropy which generate a number of astropy.Time objects then perform a number of tests (including writing and reading the times to asdf):
https://github.com/astropy/asdf-astropy/blob/main/asdf_astropy/converters/time/tests/test_time.py

As not all NDCube objects are currently supported, it would be helpful to have tests for the unsupported objects that check for helpful errors raised by the converter.

[ViciousEagle03]

@ViciousEagle03 Would you add some tests to this PR that verify that a variety of NDCube objects can be written to and read from an asdf file? For some inspiration, here are some examples in asdf-astropy which generate a number of astropy.Time objects then perform a number of tests (including writing and reading the times to asdf): https://github.com/astropy/asdf-astropy/blob/main/asdf_astropy/converters/time/tests/test_time.py

As not all NDCube objects are currently supported, it would be helpful to have tests for the unsupported objects that check for helpful errors raised by the converter.

I've added GWCS objects and tests to check the roundtrip serialization of the NDCube. If I have missed something, please suggest any additional tests to improve the coverage.

[braingram]

I'll try to review this today. My general impression is it looks great and I'm curious to hear thoughts from someone who has more (I should say any) real-world ndcube experience and can try saving and loading a few compatible ndcubes. @Cadair any suggestions for a suitable test-subject?

[braingram]

I left some minor comments. Overall looks good to me! There are a few comments about converters and schemas I'd like to resolve before approving but I suspect the resolution will be quick. Nice work!

[ViciousEagle03]

I have not added the extra_coords with SkyCoordTableCoordinate because there seems to be unexpected behaviour when using assert_extra_coords_equal to verify the roundtrip serialization.

[braingram]

I'm a bit confused. Several of the suggestions in my previous comments were marked as resolved with responses that gave me the impression they were "ok" yet the changes were not made. I was going through updating comments but stopped when I realized this PR might not be up-to-date.

Are there changes not committed that have the previous suggestions? If not, is there a reason these suggestions were dismissed?

EDIT: I'm not sure what's up with github but me posting this comment caused the PR to update for me :-/ I'm now seeing the changes made in cd31d95 which I did not previously see.

[braingram]

Now that I'm looking at the newest code 🤦

Just a few comments/questions and one request.

Would you add asdf to the dependencies and set a version that makes the oldestdeps job happy?

[ViciousEagle03]

Now that I'm looking at the newest code 🤦

Just a few comments/questions and one request.

Would you add asdf to the dependencies and set a version that makes the oldestdeps job happy?

Sure, I am on it.

[ViciousEagle03]

Apologies for the delay @braingram. To ensure the gwcs tests pass, we need to use at least version 0.20.0 of gwcs because it includes the necessary information( The addition of pixel_shape to the wcs schema used during the roundtrip serialization) introduced in that version. gwcs version 0.20.0 requires a minimum of astropy version 5.3, and the asdf version that passes the tests is at least 2.14.4.

[braingram]

Looks good to me. Thanks!