Fits wcs by braingram · Pull Request #246 · astropy/asdf-astropy

braingram · 2024-11-05T19:03:31Z

This PR builds off of #235
and works around an astropy issue where SIP coefficients lose precision: astropy/astropy#17334

codecov · 2024-11-05T19:13:02Z

Codecov Report

Attention: Patch coverage is 99.38650% with 1 line in your changes missing coverage. Please review.

Project coverage is 99.40%. Comparing base (5a2f7cf) to head (1edecd1).
Report is 52 commits behind head on main.

Files with missing lines	Patch %	Lines
asdf_astropy/testing/helpers.py	95.65%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #246      +/-   ##
==========================================
- Coverage   99.45%   99.40%   -0.06%     
==========================================
  Files          57       62       +5     
  Lines        2037     2200     +163     
==========================================
+ Hits         2026     2187     +161     
- Misses         11       13       +2

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

braingram · 2024-11-05T20:23:07Z

asdf_astropy/resources/manifests/astropy-1.2.0.yaml

  model classes, which are handled by an implementation of the ASDF
  transform extension.
 asdf_standard_requirement:
-  gte: 1.6.0


Note to reviewers this manifest requires ASDF standard 1.6.0 which has not yet become the stable release. Since this PR adds new features that are compatible with the current stable ASDF standard 1.5.0 I chose to change this manifest (astropy-1.2.0) to instead support the stable version (using all the pre 1.6.0 schemas) and adding the new WCS schemas.

I also added astropy-1.3.0 which support 1.6.0 and also adds the new WCS schemas for that standard version.

So to summarize. Before this PR:

astropy-1.2.0 supported ASDF standard 1.6.0 (the unused development version)

After this PR:

astropy-1.2.0 supports ASDF standard 1.5.0 and adds the WCS schemas

astropy-1.3.0 supports ASDF standard 1.6.0 and adds the WCS schemas

braingram · 2024-11-05T20:47:03Z

@Cadair I can't request you as a reviewer. Would you give this a review?

braingram · 2024-11-05T20:48:59Z

@ViciousEagle03 I put together this PR based on the work you did serializing astropy.wcs.WCS. It turns out one of the hang-ups is a bug in astropy (that was a fun one to track down). If you're available to give this a review that would be great! Thanks again for all your work!

ViciousEagle03 · 2024-11-06T00:52:19Z

@ViciousEagle03 I put together this PR based on the work you did serializing astropy.wcs.WCS. It turns out one of the hang-ups is a bug in astropy (that was a fun one to track down). If you're available to give this a review that would be great! Thanks again for all your work!

This is great! Thanks for cleaning up the code and finding a workaround for the bug. We can finally merge the rest of the PRs dependent on the serialization of astropy.wcs.WCS .

Cadair · 2024-11-06T09:12:49Z

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

+            - type: integer
+            - type: object


Would it be simpler if we always stored a slice object with stop = start + 1?

Possibly. Although I think this should retain the format of the _slices_array. Which can contain all of these values.

That's fair, let's leave it as is.

Cadair · 2024-11-06T09:14:47Z

asdf_astropy/converters/wcs/tests/test_wcs.py

+    return wcs
+
+
+def create_tabular_wcs():


This is a distortion table, but do we have a test for a -TAB wcs?

Any suggestions for an example to try?

If I use the one here:
https://github.com/astropy/astropy/blob/0f80be718107ef797a58ba42025e70b21c454c61/astropy/wcs/tests/conftest.py#L11
it doesn't roundtrip through pickle (or to_fits)

import pickle from astropy.wcs import WCS from astropy.wcs.tests.helper import SimModelTAB st = SimModelTAB(nx=150, ny=200) hdulist = st.hdulist wcs = WCS(hdulist[0].header, hdulist) pickle.loads(pickle.dumps(wcs))

KeyError: "Extension ('WCS-TABLE', 1) not found."

The file in the astropy package data get_pkg_data_filename("data/tab-time-last-axis.fits", package="astropy.wcs.tests") also fails to round-trip through pickle (or to_fits).

Oh astropy/astropy#9998 astropy can't produce a -TAB wcs so we can't support it here.

Well I guess we should write a test which errors then 😆

pllim · 2024-11-06T15:24:36Z

tl;dr -- Isn't the whole point of ASDF is to be able to use GWCS so you can ditch FITS WCS?

Cadair · 2024-11-06T16:44:16Z

The point of ASDF is to be able to save things, this work is part of a GSOC project for saving NDCube objects to ASDF, if you have backed your NDCube with a gwcs or an astropy WCS it shouldn't matter you should be able to save it to and load it from ASDF.

braingram · 2024-11-07T17:46:52Z

5f34952 adds some tests using the astropy.wcs.tests.data. I wasn't able to find a definitive list of "wcses that should work" and some of the test data is designed to trigger errors. I tried to get most of the files that were not designed to trigger errors.

This did reveal one bug in this PR where setting "pixel_shape" on deserialization resulted in an error for https://github.com/astropy/astropy/blob/7896e82c0b06ea4b3870711e7124569f812df39f/astropy/wcs/tests/data/spectra/orion-velo-1.hdr since the "pixel_shape" is inconsistent with "naxis". I fixed this by removing the "pixel_shape" setting (and looking back I don't see this set anywhere in WCS_unpickle which was the model for this PR).

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

perrygreenfield

LGTM, as much as FITS WCS looks good to me ;-)

braingram force-pushed the fits_wcs branch from 60b029b to b1d8ccb Compare November 5, 2024 19:08

braingram commented Nov 5, 2024

View reviewed changes

braingram marked this pull request as ready for review November 5, 2024 20:46

braingram requested review from nden and perrygreenfield November 5, 2024 20:46

Cadair reviewed Nov 6, 2024

View reviewed changes

Cadair approved these changes Nov 6, 2024

View reviewed changes

Cadair mentioned this pull request Nov 6, 2024

Add support for ASDF serialisation and deserialisation sunpy/ndcube#776

Merged

11 tasks

This was referenced Nov 6, 2024

Add serialization logic for uncertainty objects #239

Merged

Support serialization of astropy.wcs.WCS objects to ASDF. #235

Closed

ViciousEagle03 and others added 12 commits November 13, 2024 08:12

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

c995400

Style Fix

1272c5d

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

880ff6d

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

Add SlicedLowLevelWCS serialization logic

a77c229

Remove unnecessary check

45fd10a

Apply suggestions from code review

4d04f5b

Update schema for fitswcs and slicedwcs instance

2e37f2a

Fix tests and schema

5afd469

Add changelog

54a33da

Revert small change

7262e0d

Apply review suggestion

3e2cb9d

Add to a manifest and register it as a new extension

c0872c2

ViciousEagle03 and others added 18 commits November 13, 2024 08:14

Small change

83ff33f

Correct the test suite of table lookup distortion wcs

706f870

Apply code suggestions

aab79f9

correct the standard version

160532f

move fitswcs and sliced_wcs converters to wcs submodule

5213d4e

clean up warnings, fix sip

c324e00

rename hdu to hdulist

cb69b81

reorganize manifests

dc200c7

update PR number

c9d441e

slicedwcs converter cleanup

b139108

rename fitswcs to wcs and put in wcs with slicedwcs

4bd4c2c

minor schema docs

2f2cb9d

add assert_wcs_equal, reorg tests

2e5973b

rename

b20650f

use assert_wcs_equal for sliced tests

160adcc

add attrs and blank wcs test

93b946e

fix pixel_shape roundtrip bug, test against astropy test data

8c6231b

cleanup after rebase

1edecd1

braingram force-pushed the fits_wcs branch from 5f34952 to 1edecd1 Compare November 13, 2024 13:25

perrygreenfield approved these changes Nov 13, 2024

View reviewed changes

braingram merged commit 0cacd74 into astropy:main Nov 13, 2024

braingram deleted the fits_wcs branch November 13, 2024 14:12

braingram mentioned this pull request Nov 13, 2024

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

Closed

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

braingram commented Nov 5, 2024

Uh oh!

braingram commented Nov 5, 2024

Uh oh!

ViciousEagle03 commented Nov 6, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Nov 6, 2024

Uh oh!

Cadair commented Nov 6, 2024

Uh oh!

braingram commented Nov 7, 2024

Uh oh!

perrygreenfield left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

braingram commented Nov 5, 2024 •

edited

Loading

codecov bot commented Nov 5, 2024 •

edited

Loading