astropy icon indicating copy to clipboard operation
astropy copied to clipboard

Add a new keyword argument to the WCS class to control the SIP parsing behavior

Open astrofrog opened this issue 3 years ago • 3 comments

Description

Currently when parsing the WCS of a FITS file that has SIP coefficients in the header but no -SIP suffix in the CTYPE, we emit an INFO message:

In [3]: wcs = WCS(header)
INFO: 
                Inconsistent SIP distortion information is present in the FITS header and the WCS object:
                SIP coefficients were detected, but CTYPE is missing a "-SIP" suffix.
                astropy.wcs is using the SIP distortion coefficients,
                therefore the coordinates calculated here might be incorrect.

                If you do not want to apply the SIP distortion coefficients,
                please remove the SIP coefficients from the FITS header or the
                WCS object.  As an example, if the image is already distortion-corrected
                (e.g., drizzled) then distortion components should not apply and the SIP
                coefficients should be removed.

                While the SIP distortion coefficients are being applied here, if that was indeed the intent,
                for consistency please append "-SIP" to the CTYPE in the FITS header or the WCS object.
                 [astropy.wcs.wcs]

The solutions above involve modifying the FITS file, which obviously is ideal but not always easy for users to do. This PR proposes to add a new keyword argument that will make it easier to control the desired behavior. With this PR the message is tweaked:

In [3]: wcs = WCS(header)
INFO: 
                Inconsistent SIP distortion information is present in the FITS header and the
                WCS object: SIP coefficients were detected, but CTYPE is missing a "-SIP"
                suffix. astropy.wcs is using the SIP distortion coefficients, therefore the
                coordinates calculated here might be incorrect.

                If you do not want to apply the SIP distortion coefficients, you can pass
                ``sip_requires_ctype_prefix=True`` to ``WCS``. Alternatively, you should remove
                the SIP coefficients from the FITS header or the WCS object. As an example, if
                the image is already distortion-corrected (e.g., drizzled) then distortion
                components should not apply and the SIP coefficients should be removed.

                If you do intend to use the SIP coefficients and want to avoid seeing this
                message, you can pass ``sip_requires_ctype_prefix=False`` to ``WCS``, or if you
                can, you should append "-SIP" to the CTYPE in the FITS header or the WCS
                object.
                 [astropy.wcs.wcs]

and when using the keyword argument, the message is never shown:

In [3]: wcs = WCS(header, sip_requires_ctype_prefix=False)
WARNING: FITSFixedWarning: 'datfix' made the change 'Set MJD-OBS to 52680.000000 from DATE-OBS'. [astropy.wcs.wcs]

In [4]: wcs.sip
Out[4]: <astropy.wcs.Sip at 0x7fb27eefd0a0>

In [5]: wcs = WCS(header, sip_requires_ctype_prefix=True)
WARNING: FITSFixedWarning: 'datfix' made the change 'Set MJD-OBS to 52680.000000 from DATE-OBS'. [astropy.wcs.wcs]

In [6]: wcs.sip

In [7]: 

This still needs updates to the docs and a test, so it will likely not be ready for 5.1, but @mcara @nden I am curious what you think about this?

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • [ ] Do the proposed changes actually accomplish desired goals?
  • [ ] Do the proposed changes follow the Astropy coding guidelines?
  • [ ] Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • [ ] Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • [ ] Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
  • [ ] Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the Extra CI label.
  • [ ] Is a change log needed? If yes, did the change log check pass? If no, add the no-changelog-entry-needed label. If this is a manual backport, use the skip-changelog-checks label unless special changelog handling is necessary.
  • [ ] Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • [ ] Is a milestone set? Milestone must be set but astropy-bot check might be missing; do not let the green checkmark fool you.
  • [ ] At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate backport-X.Y.x label(s) before merge.

astrofrog avatar Apr 22 '22 15:04 astrofrog

What is wrong with my suggestion in the original issue to not load SIP for non-celestial axes?

To be clear, I intended for this keyword to be useful even in the case of celestial axes - I just thought that this would at least allow the intended/desired behavior to be explicit.

astrofrog avatar Apr 22 '22 16:04 astrofrog

I understand. However my issue is with

this would at least allow the intended/desired behavior to be explicit

"intended" and "desired" behavior is actually not explicit from the "archival" point of view. It is known only to the caller of the function (and only to those callers that actually know how the file was intended to be used) and it is not contained in the FITS file itself.

mcara avatar Apr 22 '22 16:04 mcara

Hi humans :wave: - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

github-actions[bot] avatar Sep 20 '22 06:09 github-actions[bot]