Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 6 commits into
base: asdf-support
Choose a base branch
from

Conversation

ViciousEagle03
Copy link
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
Copy link
Member Author

ViciousEagle03 commented Aug 16, 2024

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
Copy link

braingram commented Aug 17, 2024

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
Copy link
Member

Cadair commented Aug 22, 2024

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
Copy link
Member

@Cadair Cadair left a comment

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 👍

ndcube/asdf/converters/reorderedwcs_converter.py Outdated Show resolved Hide resolved
ndcube/asdf/converters/resampled_converter.py Outdated Show resolved Hide resolved
Comment on lines 16 to 19
factor:
type: object
offset:
type: object
Copy link
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?

Copy link
Member Author

@ViciousEagle03 ViciousEagle03 Aug 23, 2024

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
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.

if ndim == 1:
TabularND = Tabular1D
elif ndim == 2:
TabularND = Tabular2D
Copy link
Member

Choose a reason for hiding this comment

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

I think this should have an else for >2D. Also a comment saying that this is for asdf would be good for future reference.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

@ViciousEagle03
Copy link
Member Author

ViciousEagle03 commented Aug 31, 2024

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?

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.

3 participants