Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-2883

wcslib is unable to read PTF headers with PV1_{1..16} cards

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Labels:
      None

      Description

      SCAMP writes distortion headers in form of PVi_nn (i=1..x, nn=5..16) cards, but this is rejected (correctly) by wcslib 4.14; there is a discussion at https://github.com/astropy/astropy/issues/299

      The simplest "solution" is to strip the values PV1_nn (nn=5..16) in makeWcs() for CTYPEs of TAN or TAN-SIP and this certainly works.

      I propose that we adopt this solution for now.

        Attachments

          Issue Links

            Activity

            Hide
            tjenness Tim Jenness added a comment -

            It seems that in 2011 the SCAMP team agreed to stop using -TAN for their distorted WCS as it was clearly breaking the standard (well, it was part of the draft standard described in paper II in 2000). They agreed to use -TPV instead. Are PTF issuing new files using -TAN?

            David Berry reports:

            The FitsChan class in AST handles this as follows:

            1) If the CTYPE in a FITS header uses TPV, then the the PVi_j headers are interpreted according to the conventions of the distorted TAN paper above.

            2) For CTYPEs that use TAN, the interpretation of PVi_j values is controlled by the "PolyTan" attribute of the FitsChan. This can be set to an explicit value before reading the header to indicate the convention to use. If it is not set before reading the header, a heuristic is used to guess the most appropriate convention as follows:

            If the FitsChan contains any PVi_m keywords for the latitude axis, or if it contains PVi_m keywords for the longitude axis with "m" greater than 4, then the distorted TAN convention is used. Otherwise, the standard convention is used.

            This is relatively safe since the published FITS-WCS does not define/allow any PV terms to be associated with the latitude axis of a standard TAN projection. So if a header uses TAN and there are some PV terms attached to the latitude axis, then it's pretty safe to assume we have a SCAMP-like header.

            In addition, the standard only allows/defines PVi_j values up to PVi_4 on the longitude axis of any projection, and all the SCAMP headers I have seen use more than 4 PV terms on the longitude axis - again another give-away that the standard is not being used.

            BTW - the only common usage of the longitude PV terms reserved by the standard is as an alternative to using LONPOLE and LATPOLE. They can also be used to decouple CRVAL from CRPIX (see FITS-WCS paper 2 section 2.5) but I've never seen that facility used in practice.

            end quote

            So note that if you strip the higher PV keywords from a header that uses -TAN then you are obviously not going to get a good WCS since none of the distortion terms will be in play. If AST sees -SIP it will just assume it's a SIP header and not worry about anything else.

            All these heuristics are why the FITS-WCS parsing code in AST is more than 40,000 lines. WCSLIB is not in the game of handling WCS outside of the FITS standard. You can actually use AST to normalize the WCS in a FITS header: you throw the header at AST and then ask it to regenerate the WCS keywords in a more standards compliant form. That is much more robust than an external user trying to hack at the header to get it in to a useful place for WCSLIB.

            Show
            tjenness Tim Jenness added a comment - It seems that in 2011 the SCAMP team agreed to stop using -TAN for their distorted WCS as it was clearly breaking the standard (well, it was part of the draft standard described in paper II in 2000). They agreed to use -TPV instead. Are PTF issuing new files using -TAN? David Berry reports: The FitsChan class in AST handles this as follows: 1) If the CTYPE in a FITS header uses TPV, then the the PVi_j headers are interpreted according to the conventions of the distorted TAN paper above. 2) For CTYPEs that use TAN, the interpretation of PVi_j values is controlled by the "PolyTan" attribute of the FitsChan. This can be set to an explicit value before reading the header to indicate the convention to use. If it is not set before reading the header, a heuristic is used to guess the most appropriate convention as follows: If the FitsChan contains any PVi_m keywords for the latitude axis, or if it contains PVi_m keywords for the longitude axis with "m" greater than 4, then the distorted TAN convention is used. Otherwise, the standard convention is used. This is relatively safe since the published FITS-WCS does not define/allow any PV terms to be associated with the latitude axis of a standard TAN projection. So if a header uses TAN and there are some PV terms attached to the latitude axis, then it's pretty safe to assume we have a SCAMP-like header. In addition, the standard only allows/defines PVi_j values up to PVi_4 on the longitude axis of any projection, and all the SCAMP headers I have seen use more than 4 PV terms on the longitude axis - again another give-away that the standard is not being used. BTW - the only common usage of the longitude PV terms reserved by the standard is as an alternative to using LONPOLE and LATPOLE. They can also be used to decouple CRVAL from CRPIX (see FITS-WCS paper 2 section 2.5) but I've never seen that facility used in practice. end quote So note that if you strip the higher PV keywords from a header that uses -TAN then you are obviously not going to get a good WCS since none of the distortion terms will be in play. If AST sees -SIP it will just assume it's a SIP header and not worry about anything else. All these heuristics are why the FITS-WCS parsing code in AST is more than 40,000 lines. WCSLIB is not in the game of handling WCS outside of the FITS standard. You can actually use AST to normalize the WCS in a FITS header: you throw the header at AST and then ask it to regenerate the WCS keywords in a more standards compliant form. That is much more robust than an external user trying to hack at the header to get it in to a useful place for WCSLIB.
            Hide
            rhl Robert Lupton added a comment -

            Kian-Tat Lim That's why I don't want to follow astropy (we should get them to fix their code to preserve PV1_1..4).

            In the short term, my proposal to strip higher PV1 cards from TAN and TAN-SIP reduces to doing the same thing as AST, except that we ignore rather than support the TPV distortions. In another ticket we could add support for TPV (but see the end of this comment for another possibility).

            So note that if you strip the higher PV keywords from a header that uses -TAN then you are obviously not going to get a good WCS since none of the distortion terms will be in play. If AST sees -SIP it will just assume it's a SIP header and not worry about anything else.

            Ignoring the higher terms shouldn't be a disaster as it's required to decay to the best fit affine transform. But in this case, we fallback to using the SIP distortion terms so we're fine.

            So what should we do in the longer run? I agree with KT that adding tweak after tweak for legacy headers is undesirable, although probably not too bad. The alternative seems to be AST, although I am still very nervous about adopting the AST pseudo-C++ codebase, and would like an estimate of what it'd take to convert it to real C++ — my guess is not all that much, and that it could be done by someone without domain knowledge. I am also not sure that I want us to own all of AST (e.g. the plotting).

            Show
            rhl Robert Lupton added a comment - Kian-Tat Lim That's why I don't want to follow astropy (we should get them to fix their code to preserve PV1_1..4). In the short term, my proposal to strip higher PV1 cards from TAN and TAN-SIP reduces to doing the same thing as AST, except that we ignore rather than support the TPV distortions. In another ticket we could add support for TPV (but see the end of this comment for another possibility). So note that if you strip the higher PV keywords from a header that uses -TAN then you are obviously not going to get a good WCS since none of the distortion terms will be in play. If AST sees -SIP it will just assume it's a SIP header and not worry about anything else. Ignoring the higher terms shouldn't be a disaster as it's required to decay to the best fit affine transform. But in this case, we fallback to using the SIP distortion terms so we're fine. So what should we do in the longer run? I agree with KT that adding tweak after tweak for legacy headers is undesirable, although probably not too bad. The alternative seems to be AST, although I am still very nervous about adopting the AST pseudo-C++ codebase, and would like an estimate of what it'd take to convert it to real C++ — my guess is not all that much, and that it could be done by someone without domain knowledge. I am also not sure that I want us to own all of AST (e.g. the plotting).
            Hide
            rhl Robert Lupton added a comment -

            I've added the same logic as AST (TAN + PVi_j => TPV), and fixed the code that used to silently convert TAN to TPV (it now logs a message and removes
            the offending PVi_j cards).

            Note that we assume that CTYPE1 is the longitude (in this case RA) axis, so it's PV1_

            {1..4}

            that are legal. We won't handle CTYPE1=="DEC--TAN" + PVi_j correctly; but only SDSS switches RA and DEC (I hope)

            Show
            rhl Robert Lupton added a comment - I've added the same logic as AST (TAN + PVi_j => TPV), and fixed the code that used to silently convert TAN to TPV (it now logs a message and removes the offending PVi_j cards). Note that we assume that CTYPE1 is the longitude (in this case RA) axis, so it's PV1_ {1..4} that are legal. We won't handle CTYPE1=="DEC--TAN" + PVi_j correctly; but only SDSS switches RA and DEC (I hope)
            Hide
            tjenness Tim Jenness added a comment -

            The patch looks reasonable. I had one comment on Github that wasn't a new problem.

            I'd like to make the commit message more explicit about what is happening. Broken --TAN is being converted to TPV. TPV is not (yet) supported by LSST software so TPV is then converted back to --TAN.

            The discussion in the ticket mentions SIP but I think the outcome of this patch is that wcsLib now supports SIP anyhow and will ignore the additional PV items in that case. Does this require wcsLib to be updated? Or did it turn out that all the files we were worried about where broken --TAN format?

            Show
            tjenness Tim Jenness added a comment - The patch looks reasonable. I had one comment on Github that wasn't a new problem. I'd like to make the commit message more explicit about what is happening. Broken --TAN is being converted to TPV. TPV is not (yet) supported by LSST software so TPV is then converted back to --TAN. The discussion in the ticket mentions SIP but I think the outcome of this patch is that wcsLib now supports SIP anyhow and will ignore the additional PV items in that case. Does this require wcsLib to be updated? Or did it turn out that all the files we were worried about where broken --TAN format?
            Hide
            rhl Robert Lupton added a comment -

            I improved the commit message as requested, and merged to master

            Show
            rhl Robert Lupton added a comment - I improved the commit message as requested, and merged to master

              People

              Assignee:
              rhl Robert Lupton
              Reporter:
              rhl Robert Lupton
              Reviewers:
              Tim Jenness
              Watchers:
              Kian-Tat Lim, Robert Lupton, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.