Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.39%. Comparing base (ae55fed) to head (97556cc).
⚠️ Report is 31 commits behind head on main.

@@           Coverage Diff           @@
##             main     #276   +/-   ##
=======================================
  Coverage   99.39%   99.39%           
=======================================
  Files          64       65    +1     
  Lines        2309     2326   +17     
=======================================
+ Hits         2295     2312   +17     
  Misses         14       14           

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

I don't know what to do about these test fails. Some of the test headers we are using from astropy have some missing NAXISn keys, one in particular has NAXIS1 but not 2-4. This means that the pixel shape doesn't roundtrip, because astropy sets pixel_shape without validation when reading the header apparently, but not when we explicitly set it using the setter.

Interestingly, this did not happen when we were using the private _naxis API because that went through the header. I propose we go back to _naxis as I think it's actually the least worst option, and I'll go open some issues on astropy about all this and hopefully we can get it sorted in the future?

I don't know what to do about these test fails. Some of the test headers we are using from astropy have some missing NAXISn keys, one in particular has NAXIS1 but not 2-4. This means that the pixel shape doesn't roundtrip, because astropy sets pixel_shape without validation when reading the header apparently, but not when we explicitly set it using the setter.

Interestingly, this did not happen when we were using the private _naxis API because that went through the header. I propose we go back to _naxis as I think it's actually the least worst option, and I'll go open some issues on astropy about all this and hopefully we can get it sorted in the future?

Hmmm... taking https://github.com/astropy/astropy/blob/main/astropy/wcs/tests/data/spectra/orion-freq-1.hdr as an example.

from astropy.wcs import WCS
from astropy.utils.data import get_pkg_data_filename
fn = get_pkg_data_filename("tests/data/spectra/orion-wave-1.hdr", "astropy.wcs")
wcs = WCS(open(fn).read())
wcs.pixel_shape = wcs.pixel_shape

produces 2 warnings and an error:

WARNING: FITSFixedWarning: The WCS transformation has more axes (4) than the image it is associated with (1) [astropy.wcs.wcs]
WARNING: FITSFixedWarning: 'datfix' made the change 'Set MJD-OBS to 53925.853472 from DATE-OBS'. [astropy.wcs.wcs]

Error:

ValueError: The number of data axes, 4, does not equal the shape 2.

Based on the header what's the expectation here for pixel_shape?

I opened a PR against this PR:
Cadair#1

Let me know what you think.

Thanks @braingram I did consider that option, but I wasn't sure if you wanted to yank all the spectral WCSes. We should definitely open an issue upstream about the invalid headers.

What other issues should we open upstream?

• That pixel_shape can be set to an invalid value.
• That NAXISn isn't round-tripped through a header.

Thanks @braingram I did consider that option, but I wasn't sure if you wanted to yank all the spectral WCSes.

I'm ok dropping them given their odd behavior after loading. Maybe we can re-add them in the future if they're updated.

We should definitely open an issue upstream about the invalid headers.

What other issues should we open upstream?

* That `pixel_shape` can be set to an invalid value.

* That `NAXISn` isn't round-tripped through a header.

I'm not familiar enough with astropy WCS to know what's considered an issue.

@braingram can we merge this?

No objection from me. Merge away or let me know and I'll merge it.