Skip to content

Add support for the serialization of the ndcube WCS wrappers.#751

Merged
Cadair merged 6 commits into
sunpy:asdf-supportfrom
ViciousEagle03:wcs_wrapper_asdf_support
Nov 6, 2024
Merged

Add support for the serialization of the ndcube WCS wrappers.#751
Cadair merged 6 commits into
sunpy:asdf-supportfrom
ViciousEagle03:wcs_wrapper_asdf_support

Conversation

@ViciousEagle03

Copy link
Copy Markdown
Member

This PR adds the support for the serialization of the following wrapper class:

ResampledLowLevelWCS
CompoundLowLevelWCS
ReorderedLowLevelWCS

Currently, this PR does not have tests written with underlying WCS attributes as the astropy.wcs.WCS and would be added after the asdf-astropy PR astropy/asdf-astropy#235 is merged.

Fixes: #750


Note: This PR Should be merged after #708 is merged

@ViciousEagle03

ViciousEagle03 commented Aug 16, 2024

Copy link
Copy Markdown
Member Author

Hi, @braingram, during the Tox CI/CD testing, the test for the test_serialization_compoundwcs is [failing]

It is throwing an asdf.exceptions.AsdfSerializationError indicating that the object of type astropy.modeling.tabular.Tabular1D is not serializable.

I checked the env created by the Tox, and it has asdf-astropy version as 0.6.1, which supports the serialization of this object by the tag: tag:stsci.edu:asdf/transform/tabular-1*.

@braingram

braingram commented Aug 17, 2024

Copy link
Copy Markdown
Contributor

It looks like _generate_tabular calls tabular_model:

TabularND = tabular_model(ndim, name=f"Tabular{ndim}D")

This creates a new class, one that hasn't been registered with the asdf-astropy Tabular1D converter. So in this case, even though the class "looks" the same it's unknown to asdf. A minimal example that shows this is:

>> import astropy.modeling.tabular
>> t1d = astropy.modeling.tabular.tabular_model(1, name="Tabular1D")
>> t1d
<class 'astropy.modeling.tabular.Tabular1D'>
Name: Tabular1D
N_inputs: 1
N_outputs: 1
>> astropy.modeling.tabular.Tabular1D
<class 'astropy.modeling.tabular.Tabular1D'>
Name: Tabular1D
N_inputs: 1
N_outputs: 1
>> t1d is not astropy.modeling.tabular.Tabular1D
True

Let me think about how to fix this. There are issues supporting dynamic classes in asdf and the extension machinery is not set up to support this pattern. If it's possible to update table_coord.py to use the existing Tabular1D and Tabular2D for those ndim values the wcses can be supported by the existing machinery.

@Cadair

Cadair commented Aug 22, 2024

Copy link
Copy Markdown
Member

If it's possible to update table_coord.py to use the existing Tabular1D and Tabular2D for those ndim values the wcses can be supported by the existing machinery.

This should be easy enough.

@Cadair Cadair added this to the asdf milestone Aug 22, 2024
@Cadair Cadair added the No Changelog Entry Needed Skip all changelog checks. label Aug 22, 2024

@Cadair Cadair left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Other than the test issue, I think the code looks generally good 👍

Comment thread ndcube/asdf/converters/reorderedwcs_converter.py Outdated
Comment thread ndcube/asdf/converters/resampled_converter.py Outdated
Comment on lines +16 to +19
factor:
type: object
offset:
type: object

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These can just be numbers?

@ViciousEagle03 ViciousEagle03 Aug 23, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, these values can be numbers, but since there isn't any public method to retrieve the values of factor and offset, we have to access the private attributes _factor and _offset. These attributes are instances of the np.array class(factor and offset are converted to numpy array here), so the schema checks if they are of type "object."

However, it might be better to check against the specific tag: "tag:stsci.edu:asdf/core/ndarray-1.0.0". Thanks for pointing it out.

@ViciousEagle03

Copy link
Copy Markdown
Member Author

If it's possible to update table_coord.py to use the existing Tabular1D and Tabular2D for those ndim values the wcses can be supported by the existing machinery.

This should be easy enough.

I am on it.

@ViciousEagle03 ViciousEagle03 requested a review from Cadair August 25, 2024 10:42
Comment thread ndcube/extra_coords/table_coord.py
@ViciousEagle03

ViciousEagle03 commented Aug 31, 2024

Copy link
Copy Markdown
Member Author

Hey @Cadair and @braingram, is the PR ready to be merged, or is there anything else that needs to be addressed besides extending the test suite when the astropy/asdf-astropy#235 PR is merged?

Comment thread ndcube/asdf/converters/tests/test_ndcube_wcs_wrappers.py
Comment thread ndcube/asdf/converters/compoundwcs_converter.py Outdated
Comment thread ndcube/asdf/converters/compoundwcs_converter.py Outdated
Comment thread ndcube/asdf/converters/reorderedwcs_converter.py Outdated
Comment thread ndcube/asdf/converters/reorderedwcs_converter.py Outdated
Comment thread ndcube/asdf/converters/resampled_converter.py Outdated
Comment thread ndcube/asdf/converters/resampled_converter.py Outdated
Comment thread ndcube/asdf/resources/schemas/ndcube-0.1.0.yaml Outdated
Comment thread ndcube/asdf/resources/schemas/ndcube-0.1.0.yaml
@Cadair Cadair force-pushed the wcs_wrapper_asdf_support branch from 2ea9a67 to 4b865c4 Compare November 6, 2024 09:23
@Cadair Cadair force-pushed the wcs_wrapper_asdf_support branch from 4b865c4 to ddc75be Compare November 6, 2024 09:27
Comment thread ndcube/asdf/converters/ndcube_converter.py Outdated
Comment thread ndcube/asdf/converters/ndcube_converter.py Outdated
Comment thread ndcube/asdf/converters/reorderedwcs_converter.py Outdated
Comment thread ndcube/asdf/converters/reorderedwcs_converter.py Outdated
Comment thread ndcube/asdf/converters/reorderedwcs_converter.py
Comment thread ndcube/asdf/converters/resampled_converter.py Outdated
Comment thread ndcube/asdf/converters/tests/test_ndcube_wcs_wrappers.py Outdated
Comment thread ndcube/asdf/converters/tests/test_ndcube_wcs_wrappers.py Outdated
Comment thread ndcube/asdf/converters/tests/test_ndcube_wcs_wrappers.py
Comment thread ndcube/asdf/converters/tests/test_ndcube_wcs_wrappers.py
@Cadair Cadair merged commit 7ee5048 into sunpy:asdf-support Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

No Changelog Entry Needed Skip all changelog checks.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants