-
-
Notifications
You must be signed in to change notification settings - Fork 14
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
Support serialization of astropy.wcs.WCS
objects to ASDF.
#235
base: main
Are you sure you want to change the base?
Conversation
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 ReportAll modified and coverable lines are covered by tests ✅
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. |
Thanks for approving the CI run. |
The CI still needs to be approved to run. |
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: 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:
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 :Line 24 in 9348f89
|
Sure I am on it |
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
Hey @braingram, to enable serialization for |
I'm ok having both of those features in this PR since they seem very related. |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
@@ -28,6 +28,16 @@ tags: | |||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you add this to a new manifest astropy-1.1.0.yaml
and register it as a new extension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
astropy-1.1.0.yaml
manifest already exists. Do I need to add it to this manifest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
description: | ||
Represents the fits object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description: | |
Represents the fits object | |
description: >- | |
Represents the fits object |
Same as title
would you add a more descriptive description?
@Cadair and I had discussed using |
I did not know the FITSConverter existed, so maybe that is easier. I shall have a look in more detail tomorrow. |
Thinking about this a bit more +1 to just using |
FTR, the best solution is to change the settings in the repo settings to only require approval for new github accounts. |
from astropy.wcs import WCS | ||
|
||
|
||
def create_wcs(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there's a set of WCSes we can use to test in astropy somewhere? There has to already be one or many fixtures in astropy we can import riiighghtttt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have extracted the WCS
from the FITS
file. I thought it would be better to test directly using a FITS
file.
Hello, @braingram could you please review the recent changes. |
def from_yaml_tree(self, node, tag, ctx): | ||
from astropy.wcs import WCS | ||
|
||
primary_hdu = node["hdu"][0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@braingram, Do you have any suggestions for effectively preserving distortion data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am currently experimenting and trying to determine the appropriate serialization logic for this WCS. If you have any suggestions, please let me know.
|
||
def create_wcs(): | ||
urls = [ | ||
"http://data.astropy.org/tutorials/FITS-cubes/reduced_TAN_C14.fits", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see this
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 |
from astropy import wcs | ||
|
||
|
||
def create_sip_distortion_wcs(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I create a separate fixture for the WCS test cases, or will the existing ones work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wcses do not appear to be round-tripping.
|
||
with asdf.open(file_path) as af: | ||
loaded_wcs = af["wcs"] | ||
assert wcs.to_header() == loaded_wcs.to_header() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
@pytest.mark.filterwarnings("ignore::astropy.wcs.wcs.FITSFixedWarning") | ||
@pytest.mark.filterwarnings( | ||
"ignore:Some non-standard WCS keywords were excluded:astropy.utils.exceptions.AstropyWarning", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's triggering these warnings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
def to_yaml_tree(self, wcs, tag, ctx): | ||
node = {} | ||
if wcs.sip is not None: | ||
node["hdu"] = wcs.to_fits(relax=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I believe so.
Is there a case we are not considering? If so, could you please explain how we can account for it?
return [wcs0, wcs1, wcs2, wcs_ellipsis, wcs3] | ||
|
||
|
||
@pytest.mark.filterwarnings("ignore::astropy.wcs.wcs.FITSFixedWarning") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's triggering this warning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the above reason
|
||
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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
asdf_standard_requirement: | ||
gte: 1.5.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why restrict the standard here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tox tests seem to fail with the 1.6.0
version.
items: | ||
- oneOf: | ||
- type: integer | ||
- type: object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- type: object | |
- type: object |
This needs more details. It appears to be an object with start
stop
step
all either int or None.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I will include it in the schema
@@ -0,0 +1,34 @@ | |||
from asdf.extension import Converter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, moving them to a wcs
submodule makes sense and will keep things organized.
@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. |
This PR is a [WIP] and adds the support for serialization of
astropy.wcs.WCS
to ASDF.fixes: #234
Checklist:
wcs.to_fits()
.