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

            No builds found.
            rhl Robert Lupton created issue -
            Hide
            ktl Kian-Tat Lim added a comment -

            Is it possible that PVi_nn for i > 1 or nn < 5 might actually be misinterpreted by a future wcslib? Wouldn't it be better to match the behavior of AstroPy instead of implementing yet another different policy?

            Show
            ktl Kian-Tat Lim added a comment - Is it possible that PVi_nn for i > 1 or nn < 5 might actually be misinterpreted by a future wcslib? Wouldn't it be better to match the behavior of AstroPy instead of implementing yet another different policy?
            Hide
            rhl Robert Lupton added a comment -

            In other words, only strip for TAN-SIP not TAN? That'd be OK; there's no good solution. Tim Jenness is investigating how AST works around this problem.

            Show
            rhl Robert Lupton added a comment - In other words, only strip for TAN-SIP not TAN? That'd be OK; there's no good solution. Tim Jenness is investigating how AST works around this problem.
            Hide
            tjenness Tim Jenness added a comment -

            I had always assumed that SCAMP used TPV (http://fits.gsfc.nasa.gov/registry/tpvwcs.html) for its TAN distortion code. Using TAN and then using odd PVs seems a bit weird.

            Show
            tjenness Tim Jenness added a comment - I had always assumed that SCAMP used TPV ( http://fits.gsfc.nasa.gov/registry/tpvwcs.html ) for its TAN distortion code. Using TAN and then using odd PVs seems a bit weird.
            Hide
            ktl Kian-Tat Lim added a comment -

            Robert Lupton Not only strip only for -SIP, but AstroPy also appears to strip all PVi_nn headers, not just the ones you suggested.

            Show
            ktl Kian-Tat Lim added a comment - Robert Lupton Not only strip only for -SIP, but AstroPy also appears to strip all PVi_nn headers, not just the ones you suggested.
            Hide
            rhl Robert Lupton added a comment -

            I don't think that's the right thing to do. Mark Calabretta's wcslib doesn't object to PV1_

            {1..4}

            , possibly because they're used in latitude/longitude calculations, e.g. Native longitude and latitude of the fiducial (calculations that I do not understand)

            Show
            rhl Robert Lupton added a comment - I don't think that's the right thing to do. Mark Calabretta's wcslib doesn't object to PV1_ {1..4} , possibly because they're used in latitude/longitude calculations, e.g. Native longitude and latitude of the fiducial (calculations that I do not understand)
            Hide
            ktl Kian-Tat Lim added a comment - - edited

            (In case there's a misunderstanding, AstroPy strips all PVi_nn headers for -SIP WCS types only, not for all WCS types.)

            In that case, I suggest we submit a pull request to AstroPy to have them be compatible with us in this regard, if we're more correct. But taking on any of these seems to inevitably lead down the slippery slope of building in more and more special case code. Where would we ever draw a line? (And what happens if/when two groups decide to use incompatible but unfortunately indistinguishable mechanisms?)

            Show
            ktl Kian-Tat Lim added a comment - - edited (In case there's a misunderstanding, AstroPy strips all PVi_nn headers for -SIP WCS types only, not for all WCS types.) In that case, I suggest we submit a pull request to AstroPy to have them be compatible with us in this regard, if we're more correct. But taking on any of these seems to inevitably lead down the slippery slope of building in more and more special case code. Where would we ever draw a line? (And what happens if/when two groups decide to use incompatible but unfortunately indistinguishable mechanisms?)
            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)
            rhl Robert Lupton made changes -
            Field Original Value New Value
            Reviewers Tim Jenness [ tjenness ]
            Status To Do [ 10001 ] In Review [ 10004 ]
            swinbank John Swinbank made changes -
            Assignee Robert Lupton [ rhl ]
            swinbank John Swinbank made changes -
            Epic Link DM-1912 [ 15945 ]
            swinbank John Swinbank made changes -
            Sprint Science Pipelines DM-S15-4 [ 159 ]
            Story Points 1
            Team Data Release Production [ 10301 ]
            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?
            tjenness Tim Jenness made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            rhl Robert Lupton made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            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
            rhl Robert Lupton made changes -
            Link This issue relates to DM-2967 [ DM-2967 ]
            yusra Yusra AlSayyad made changes -
            Link This issue relates to DM-3196 [ DM-3196 ]
            hchiang2 Hsin-Fang Chiang made changes -
            Link This issue relates to DM-4805 [ DM-4805 ]

              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 Builds

                  No builds found.