Add support for the serialization of the ndcube WCS wrappers.#751
Conversation
|
Hi, @braingram, during the Tox CI/CD testing, the test for the It is throwing an I checked the env created by the Tox, and it has |
|
It looks like ndcube/ndcube/extra_coords/table_coord.py Line 139 in 2ef42ae 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
TrueLet 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 |
This should be easy enough. |
Cadair
left a comment
There was a problem hiding this comment.
Other than the test issue, I think the code looks generally good 👍
| factor: | ||
| type: object | ||
| offset: | ||
| type: object |
There was a problem hiding this comment.
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.
I am on it. |
|
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? |
2ea9a67 to
4b865c4
Compare
…owLevelWCS CompoundLowLevelWCS
Co-authored-by: Stuart Mumford <stuart@cadair.com>
4b865c4 to
ddc75be
Compare
This PR adds the support for the serialization of the following wrapper class:
ResampledLowLevelWCSCompoundLowLevelWCSReorderedLowLevelWCSCurrently, this PR does not have tests written with underlying
WCSattributes as theastropy.wcs.WCSand would be added after theasdf-astropyPR astropy/asdf-astropy#235 is merged.Fixes: #750
Note: This PR Should be merged after #708 is merged