Ensure at least one pixel in header

[teutoburg]

Fixes #584. Not sure if there would be any unexpected negative consequences, so I included the log msg so it can be tracked down if weird things start occuring.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.79%. Comparing base (7823ad4) to head (d85c45f).
Report is 13 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #586      +/-   ##
==========================================
- Coverage   76.85%   76.79%   -0.06%     
==========================================
  Files          69       69              
  Lines        8571     8575       +4     
==========================================
- Hits         6587     6585       -2     
- Misses       1984     1990       +6     

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

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[oczoske]

The change should be alright, although I cannot really judge the consequences either. The legacy code is weird, though. In line 213, ctype is set to RA---TAN if a sky WCS is to be constructed. In line 227, the WCS keyword is set to 2 * [ctype], which means the sky WCS will have two right-ascension axes and no declination axis. That hardly makes sense and shouldn't work if the WCS is used by an actual wcs routine. Line 213 should read
ctype = ["LINEAR", "LINEAR"] if wcs_suffix in "DX" else ["RA---TAN", "DEC--TAN"]

[teutoburg]

In line 213, ctype is set to RA---TAN if a sky WCS is to be constructed. In line 227, the WCS keyword is set to 2 * [ctype], which means the sky WCS will have two right-ascension axes and no declination axis. That hardly makes sense and shouldn't work if the WCS is used by an actual wcs routine.

Yeah, the same realisation occurred to me shortly after submitting the PR. The function in question is used all the time in ScopeSim. I believe somehow assumed (without ever actually conforming this 🤦‍♂️) that the WCS interface is smart enough to change the second axis to DEC--TAN, as it also performs some other checks and transformations (unlike setting WCS head keywords manually). But it doesn't.

However, I now checked what's actually happening: The default value for the WCS suffix is an empty string (as appropriate for the 0th (sky) WCS in the header). But the check wcs_suffix in "DX"` (which is supposed to catch both "D" and "X" suffices) also is True for the empty string. So, in the (default) case of no suffix for the sky WCS, the resulting CTYPEn is "LINEAR" for both axes. Which is fine because ScopeSim still assumes a linear sky anyway. But that's still not what that function was supposed to do. So yeah, some test should have caught that 🙃

[teutoburg]

So probably something like

ctype = ["LINEAR", "LINEAR"] if wcs_suffix and wcs_suffix in "DX" else ["RA---TAN", "DEC--TAN"]

should do the trick...

[teutoburg]

Or perhaps rather

ctype = ["LINEAR", "LINEAR"] if wcs_suffix in {"D", X"} else ["RA---TAN", "DEC--TAN"]

But I'd have to check what that does for an empty string (typing on mobile, sadly no Python interpreter here...).

[teutoburg]

Further explanation about the "DX": the "D" is obviously the detector WCS in mm, as used in ScopeSim. The "X" was added by me (I believe in the context of the off-by-one fix) as a placeholder for a "LINEAR but deg/arcsec (and not mm)" WCS, that is useful to simplify test cases. As in, once ScopeSim eventually realises that is sky is, in fact, nonlinear, we can still use this key to create simplified HDUs for testing purposes.

[oczoske]

Line 205 is not needed, 208 shouldn't do any harm in any case, but it might be better to be safe.

[teutoburg]

You're right, but I wanted to include the log message to catch any potential weird cases. I thought of going with a masked array (like we did for the NaN check) and emit the log msg based on the mask, but that seemed overkill...