Support serialization of `astropy.wcs.WCS` objects to ASDF. by ViciousEagle03 · Pull Request #235 · astropy/asdf-astropy

ViciousEagle03 · 2024-07-24T15:57:23Z

This PR is a [WIP] and adds the support for serialization of astropy.wcs.WCS to ASDF.
fixes: #234
Checklist:

Add support for the basic WCSes by preserving the wcs.to_fits().
Test

braingram · 2024-07-24T15:59:29Z

FYI I had to "approve and run" the CI for this (likely because this is your first contribution). Let me know if the CI stops running and I can look into fixing it. Thanks!

codecov · 2024-07-24T16:02:09Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.97%. Comparing base (d63bc9a) to head (a016e2b).
Report is 80 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #235      +/-   ##
==========================================
+ Coverage   97.90%   97.97%   +0.07%     
==========================================
  Files          54       59       +5     
  Lines        2053     2178     +125     
==========================================
+ Hits         2010     2134     +124     
- Misses         43       44       +1

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

ViciousEagle03 · 2024-07-24T16:04:23Z

Thanks for approving the CI run.

ViciousEagle03 · 2024-07-24T16:52:54Z

The CI still needs to be approved to run.

braingram · 2024-07-24T17:16:05Z

Hmmm, I don't know of a way to "approve all runs for this PR from a first time contributor" and this suggests it might not be an option:
https://github.com/orgs/community/discussions/14334

Would it be possible to open a PR with a pretty minimal change that I can approve (which will then make you a contributor)? A PR that just removes the if check here:

asdf-astropy/asdf_astropy/converters/transform/tests/test_transform.py

Line 81 in 9348f89

if minversion("astropy", "5.1"):

and unindents the code in the if block would work. The minimum required version of astropy is now 5.2 so the if will always be True:

asdf-astropy/pyproject.toml

Line 24 in 9348f89

"astropy>=5.2.0",

ViciousEagle03 · 2024-07-24T18:13:06Z

Sure I am on it

ViciousEagle03 · 2024-08-04T19:46:30Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

ViciousEagle03 · 2024-08-04T19:51:49Z

Hey @braingram, to enable serialization for SlicedLowLevelWCS, we need the schema and converter for fitswcs. Should I push the serialization code for SlicedLowLevelWCS here, or should I add the changes into a new branch after this branch is merged?

braingram · 2024-08-05T14:14:16Z

I'm ok having both of those features in this PR since they seem very related.

braingram · 2024-08-21T15:54:56Z

asdf_astropy/converters/fits/fitswcs.py

+    Converter for serializing and deserializing `astropy.wcs.WCS` objects.
+
+    This converter currently supports the serialization of simple WCS objects
+    by preserving the `wcs.to_header()` data. It does not support complex WCS objects


Suggested change

by preserving the `wcs.to_header()` data. It does not support complex WCS objects

by preserving the ``wcs.to_header()`` data. It does not support complex WCS objects

This should hopefully fix the docs.

Since we are now using to_fits instead of to_header, we no longer need to specify that the converter supports only certain types of WCSes.

braingram · 2024-08-21T15:55:47Z

asdf_astropy/resources/manifests/astropy-1.0.0.yaml

    be accessible in its proper form in the ASDF file.

    Only image and binary table extensions are supported.
+- tag_uri: tag:astropy.org:astropy/fits/fitswcs-1.0.0


Would you add this to a new manifest astropy-1.1.0.yaml and register it as a new extension?

astropy-1.1.0.yaml manifest already exists. Do I need to add it to this manifest?

Yeah a new manifest is needed (I forgot this already has an 1.1.0).

Things are a bit more complicated as 1.2.0 exists but is only ASDF standard 1.6.0 compliant.

Would you add a new one (1.3.0) based off of the 1.1.0 one (which uses the current ASDF standard 1.5.0). I'll sort out the 1.6.0 issues after this PR is in.

asdf_astropy/resources/schemas/fits/fitswcs-1.0.0.yaml

braingram · 2024-08-21T16:12:00Z

asdf_astropy/resources/schemas/fits/fitswcs-1.0.0.yaml

+description:
+  Represents the fits object


Suggested change

description:

Represents the fits object

description: >-

Represents the fits object

Same as title would you add a more descriptive description?

ViciousEagle03 · 2024-08-25T10:00:24Z

Hello, @braingram could you please review the recent changes.

braingram · 2024-08-25T13:01:23Z

asdf_astropy/converters/fits/fitswcs.py

+    def from_yaml_tree(self, node, tag, ctx):
+        from astropy.wcs import WCS
+
+        primary_hdu = node["hdu"][0]


Suggested change

primary_hdu = node["hdu"][0]

primary_hdu = node["hdu"][0]

Doesn't this throw out the distortion (and other non-primary) data that is generated with to_fits? Would you add a test with a wcs that generates more than 1 hdu for to_fits?

@braingram, Do you have any suggestions for effectively preserving distortion data?

I believe that's already done with to_fits on write (since it puts the table into additional hdus that are stored in the asdf file with this PR).

This is all new to me so I could be getting this wrong but I think:

astropy.wcs.WCS(node["hdu"][0].header, fobj=node["hdu"])

would work for reading. Do any of the example files have distortion tables?

I am currently experimenting and trying to determine the appropriate serialization logic for this WCS. If you have any suggestions, please let me know.

braingram · 2024-08-25T13:03:09Z

asdf_astropy/converters/fits/tests/test_fitswcs.py

+
+def create_wcs():
+    urls = [
+        "http://data.astropy.org/tutorials/FITS-cubes/reduced_TAN_C14.fits",


This would introduce downloading data files as part of the test runs. This is not something done by other tests and I don't think this belongs in unit tests.

Would you add a fixture that creates suitable wcs objects?

Please see this

asdf_astropy/converters/slicedwcs/slicedwcs.py

asdf_astropy/resources/manifests/astropy-1.1.0.yaml

braingram · 2024-08-25T13:05:23Z

I left a few comments. Any clue why oldest deps is failing?

ViciousEagle03 · 2024-08-26T10:36:51Z

I left a few comments. Any clue why oldest deps is failing?

I suspect the first error is most likely due to the distortion data not being properly handled by the fitswcs converter.

ViciousEagle03 · 2024-08-28T15:57:53Z

asdf_astropy/converters/fits/tests/test_fitswcs.py

+from astropy import wcs
+
+
+def create_sip_distortion_wcs():


Should I create a separate fixture for the WCS test cases, or will the existing ones work?

braingram

The wcses do not appear to be round-tripping.

braingram · 2024-09-03T14:10:45Z

asdf_astropy/converters/fits/tests/test_fitswcs.py

+
+    with asdf.open(file_path) as af:
+        loaded_wcs = af["wcs"]
+        assert wcs.to_header() == loaded_wcs.to_header()


Suggested change

assert wcs.to_header() == loaded_wcs.to_header()

assert wcs.to_header() == loaded_wcs.to_header()

This isn't testing the distortion information for the wcses.
I tried changing this to wcs == loaded_wcs and this is failing in part due to the sip attribute not round-tripping.

The wcs == loaded_wcs appears to be defaulting to object.__eq__ (so will fail since id(wcs) != id(loaded_wcs)). This helper function could be used instead:

asdf-astropy/asdf_astropy/testing/helpers.py

Line 105 in fa5f1f4

def assert_hdu_list_equal(a, b):

and reports a failure for this test since the cards don't match:

-> assert tuple(card_a) == tuple(card_b) (Pdb) p card_a ('A_0_0', -2.0413459666638e-06, 'SIP distortion coefficient') (Pdb) p card_b ('A_0_0', -2.0413459666638133e-06, 'SIP distortion coefficient')

Since there isn't a helper function available to check if two WCS objects are equivalent, I thought of checking the round-tripping of the WCS through its header.

braingram · 2024-09-03T14:11:15Z

asdf_astropy/converters/fits/tests/test_fitswcs.py

+@pytest.mark.filterwarnings("ignore::astropy.wcs.wcs.FITSFixedWarning")
+@pytest.mark.filterwarnings(
+    "ignore:Some non-standard WCS keywords were excluded:astropy.utils.exceptions.AstropyWarning",


What's triggering these warnings?

I filtered the FITSFixedWarning because it was triggered due to the test setup where the image has 0 axes but the WCS expects NAXIS to be 4 as no image is associated with it.

The exact warning is as follows:

astropy.wcs.wcs.FITSFixedWarning: The WCS transformation has more axes (4) than the image it is associated with (0)

Since the warning is not relevant to the test so I thought of filtering it out.

The second warning was filtered as non-standard WCS keywords (here the SIP distortion coefficients) were excluded when converting the WCS object to a FITS header using to_header.

braingram · 2024-09-03T14:13:24Z

asdf_astropy/converters/fits/fitswcs.py

+    def to_yaml_tree(self, wcs, tag, ctx):
+        node = {}
+        if wcs.sip is not None:
+            node["hdu"] = wcs.to_fits(relax=True)


Suggested change

node["hdu"] = wcs.to_fits(relax=True)

node["hdu"] = wcs.to_fits(relax=True)

Presumably the relax is here for the "non-standard" wcs keywords. Will these only be present if sip is not None?

Yes, I believe so.
Is there a case we are not considering? If so, could you please explain how we can account for it?

asdf_astropy/converters/slicedwcs/slicedwcs.py

braingram · 2024-09-03T14:14:40Z

asdf_astropy/converters/slicedwcs/test/test_slicedwcs.py

+    return [wcs0, wcs1, wcs2, wcs_ellipsis, wcs3]
+
+
+@pytest.mark.filterwarnings("ignore::astropy.wcs.wcs.FITSFixedWarning")


What's triggering this warning?

Same as the above reason

braingram · 2024-09-03T14:16:23Z

asdf_astropy/converters/slicedwcs/test/test_slicedwcs.py

+
+    with asdf.open(file_path) as af:
+        loaded_sl_wcs = af["sl_wcs"]
+        assert sl_wcs._wcs.to_header() == loaded_sl_wcs._wcs.to_header()


Suggested change

assert sl_wcs._wcs.to_header() == loaded_sl_wcs._wcs.to_header()

assert sl_wcs._wcs.to_header() == loaded_sl_wcs._wcs.to_header()

If I try to use sl_wcs == loaded_sl_wcs it fails. Any idea why the wcs is not round-tripping?

braingram · 2024-09-03T14:17:51Z

asdf_astropy/resources/manifests/astropy-1.3.0.yaml

+asdf_standard_requirement:
+  gte: 1.5.0


Why restrict the standard here?

The tox tests seem to fail with the 1.6.0 version.

asdf_astropy/resources/schemas/slicedwcs/slicedwcs-1.0.0.yaml

braingram · 2024-09-03T14:33:37Z

asdf_astropy/converters/slicedwcs/slicedwcs.py

@@ -0,0 +1,34 @@
+from asdf.extension import Converter


I believe the general layout for this package is to put converters in submodules similar to the layout in astropy. Would you move this and fitswcs.py (and the corresponding tests) into a wcs submodule? Having them both contained in the new wcs submodule is sufficient (so the SlicedWCSConverter doesn't have to be further nested in a wcsapi.wrappers.sliced_wcs submodule).

Agreed, moving them to a wcs submodule makes sense and will keep things organized.

asdf_astropy/converters/fits/tests/test_fitswcs.py

ViciousEagle03 · 2024-09-18T21:55:10Z

@braingram, @Cadair I'm sorry for being unavailable for the past two weeks because of my exams and practicals. Now that they are completed, I can fully dedicate my time to completing the project.

nden · 2024-11-06T20:25:39Z

asdf_astropy/converters/wcs/fitswcs.py

+        if wcs.sip is not None:
+            node["hdu"] = wcs.to_fits(relax=True)
+        else:
+            node["hdu"] = wcs.to_fits()


Is there a harm always running with relax=True?

I don't think so. https://github.com/astropy/asdf-astropy/pull/246/files#diff-ff1eab3e17c709f13beb01b87af0c1b0c8d6cc332436c4b7c3564673ecda950aR37 has that suggestion.

nden · 2024-11-06T20:28:14Z

asdf_astropy/converters/wcs/fitswcs.py

+
+    def to_yaml_tree(self, wcs, tag, ctx):
+        node = {}
+        if wcs.sip is not None:


astropy.wcs supports 2 other type of distortions. Are these ignored on purpose? In truth I don't know of anyone else but HST using them but the call to_fits is only truly needed if the other two distortions are present because SIP distortion is only in the headers.
I suggest removing the condition that sip is present and always call wcs.to_fits(relax=True)

Are you referring to the "table" and "TAB" distortions or is there another type?

"TAB" is a non-starter since astropy can't write them: Create a FITS file from a WCS-TAB object astropy#9998

"table" is tested: https://github.com/astropy/asdf-astropy/pull/246/files#diff-4d12199a58487bbc9c9a729bcb6f8a3d6297a6547eb1510c29c6a8721986b43eR40

nden · 2024-11-06T20:35:34Z

For the tests in this PR, would it be useful to use some of the data in the astropy.wcs.tests.data directory. There is a variety of WCSs there, including some with non-sip distortion.

Also would be be helpful to add a description to the fields in the schemas? And title?

braingram · 2024-11-06T20:46:22Z

Closing in favor of #246

Add converter and schema for basic astropy.wcs.WCS objects

d7bbee8

ViciousEagle03 marked this pull request as draft July 24, 2024 15:57

ViciousEagle03 mentioned this pull request Jul 24, 2024

Add support for serialization of astropy.wcs.WCS objects to ASDF. #234

Closed

ViciousEagle03 and others added 2 commits July 24, 2024 22:15

Style Fix

11f1b27

Merge branch 'main' into FitsWCS_support

110fc4c

ViciousEagle03 marked this pull request as ready for review July 24, 2024 16:51

ViciousEagle03 marked this pull request as draft July 24, 2024 16:51

This was referenced Jul 24, 2024

Remove astropy version check #236

Closed

Remove astropy version check #237

Merged

Merge branch 'astropy:main' into FitsWCS_support

6673946

[pre-commit.ci] auto fixes from pre-commit.com hooks

faa8680

for more information, see https://pre-commit.ci

ViciousEagle03 added 2 commits August 7, 2024 21:54

Add SlicedLowLevelWCS serialization logic

5e830f1

Remove unnecessary check

9d27051

ViciousEagle03 marked this pull request as ready for review August 15, 2024 09:03

ViciousEagle03 mentioned this pull request Aug 16, 2024

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

Merged

braingram reviewed Aug 21, 2024

View reviewed changes

asdf_astropy/resources/schemas/fits/fitswcs-1.0.0.yaml Outdated Show resolved Hide resolved

braingram reviewed Aug 21, 2024

View reviewed changes

asdf_astropy/resources/schemas/fits/fitswcs-1.0.0.yaml Outdated Show resolved Hide resolved

braingram reviewed Aug 21, 2024

View reviewed changes

asdf_astropy/resources/schemas/fits/fitswcs-1.0.0.yaml Outdated Show resolved Hide resolved

braingram reviewed Aug 21, 2024

View reviewed changes

ViciousEagle03 added 4 commits August 24, 2024 04:56

Apply suggestions from code review

66904fc

Update schema for fitswcs and slicedwcs instance

a6efc4f

Fix tests and schema

1f492ac

Add changelog

4f78680

Revert small change

de41f51

braingram reviewed Aug 25, 2024

View reviewed changes

asdf_astropy/converters/slicedwcs/slicedwcs.py Show resolved Hide resolved

braingram reviewed Aug 25, 2024

View reviewed changes

asdf_astropy/resources/manifests/astropy-1.1.0.yaml Outdated Show resolved Hide resolved

ViciousEagle03 added 3 commits August 28, 2024 19:37

Apply review suggestion

21062b5

Add to a manifest and register it as a new extension

0d31b03

Small change

0919142

ViciousEagle03 requested a review from braingram August 28, 2024 15:56

ViciousEagle03 commented Aug 28, 2024

View reviewed changes

Correct the test suite of table lookup distortion wcs

6b24fe1

braingram requested changes Sep 3, 2024

View reviewed changes

braingram reviewed Sep 3, 2024

View reviewed changes

braingram requested changes Sep 3, 2024

View reviewed changes

asdf_astropy/converters/fits/tests/test_fitswcs.py Outdated Show resolved Hide resolved

Apply code suggestions

5b1d4cf

ViciousEagle03 added 2 commits September 19, 2024 03:55

correct the standard version

53df278

move fitswcs and sliced_wcs converters to wcs submodule

a016e2b

braingram mentioned this pull request Nov 5, 2024

Fits wcs #246

Merged

nden reviewed Nov 6, 2024

View reviewed changes

braingram closed this Nov 6, 2024

	by preserving the `wcs.to_header()` data. It does not support complex WCS objects
	by preserving the ``wcs.to_header()`` data. It does not support complex WCS objects

	assert wcs.to_header() == loaded_wcs.to_header()
	assert wcs.to_header() == loaded_wcs.to_header()

	node["hdu"] = wcs.to_fits(relax=True)
	node["hdu"] = wcs.to_fits(relax=True)

		return [wcs0, wcs1, wcs2, wcs_ellipsis, wcs3]


		@pytest.mark.filterwarnings("ignore::astropy.wcs.wcs.FITSFixedWarning")

	assert sl_wcs._wcs.to_header() == loaded_sl_wcs._wcs.to_header()
	assert sl_wcs._wcs.to_header() == loaded_sl_wcs._wcs.to_header()

Uh oh!

Conversation

ViciousEagle03 commented Jul 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

braingram commented Jul 24, 2024

Uh oh!

codecov bot commented Jul 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ViciousEagle03 commented Jul 24, 2024

Uh oh!

ViciousEagle03 commented Jul 24, 2024

Uh oh!

braingram commented Jul 24, 2024

Uh oh!

ViciousEagle03 commented Jul 24, 2024

Uh oh!

ViciousEagle03 commented Aug 4, 2024

Uh oh!

ViciousEagle03 commented Aug 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

braingram commented Aug 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViciousEagle03 commented Aug 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

braingram Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

braingram commented Aug 25, 2024

Uh oh!

ViciousEagle03 commented Aug 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

braingram left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViciousEagle03 commented Jul 24, 2024 •

edited

Loading

codecov bot commented Jul 24, 2024 •

edited

Loading

ViciousEagle03 commented Aug 4, 2024 •

edited

Loading

braingram Aug 26, 2024 •

edited

Loading

ViciousEagle03 Sep 18, 2024 •

edited

Loading

ViciousEagle03 Sep 18, 2024 •

edited

Loading