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

cfitsio 3.47 causes issues with afw (testReadWriteFits) and pipe_task/meas_base

    Details

    • Type: Bug
    • Status: Invalid
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw, meas_base, pipe_tasks
    • Labels:
      None
    • Team:
      Architecture

      Description

      In upgrading cftisio to a newer version, test_exposure.py fails in testing the read output:

      https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/30666/tests

      DM-21989 will be blocked by this.

      The particular part of the test that fails seems to have been exposed by work done in DM-17982 but it's likely the underlying issue is due to a change of serialization in cfitsio.

      In DM-20864 it was also discovered that in pipe_tasks, the metadata for "base_CircularApertureFlux_radii" seems to have been persisted in uppercase, so the unit test in tests/nopytest_test_coadds.py would fail. The hack to get past that was to uppercase the key.

      This all seems possibly related to HIERARCH handling in CFITSIO, in that it's likely some keywords are persisted to a file as uppercase, so when retrieving information from that file the key had to be uppercased. I suspect this may possibly be related to the previous mentioned issue in afw.

        Attachments

          Issue Links

            Activity

            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            I'd be happy to deal with the ExposureInfo side of things. I'm not very familiar with pipe_task or measurement plugins.

            I think banning lower case names (in ExposureInfo.setComponent) is the most forward-compatible solution; if we make ExposureInfo keys case-insensitive, client code may start relying on it and we won't be able to change it later.

            Show
            krzys Krzysztof Findeisen added a comment - - edited I'd be happy to deal with the ExposureInfo side of things. I'm not very familiar with pipe_task or measurement plugins. I think banning lower case names (in ExposureInfo.setComponent ) is the most forward-compatible solution; if we make ExposureInfo keys case-insensitive, client code may start relying on it and we won't be able to change it later.
            Hide
            bvan Brian Van Klaveren added a comment -

            The second issue is actually in meas_base in ApertureFlux.cc but just shows up in pipe_task, exposed in nopytest_test_coadds.py. The issue is that the aperture flux radii is lower cased. I think there's some metadata name wrangling that goes on depending on which subclass it.

             

            I think that key maybe needs to be rethought and upper-cased. I don't have any other suggestion on the naming or handling of it.

             

            This issue is likely to occur in other parts of code putting metadata into PropertySet. I don't have an idea how widespread that is.

            Show
            bvan Brian Van Klaveren added a comment - The second issue is actually in meas_base in ApertureFlux.cc but just shows up in pipe_task, exposed in nopytest_test_coadds.py . The issue is that the aperture flux radii is lower cased. I think there's some metadata name wrangling that goes on depending on which subclass it.   I think that key maybe needs to be rethought and upper-cased. I don't have any other suggestion on the naming or handling of it.   This issue is likely to occur in other parts of code putting metadata into PropertySet. I don't have an idea how widespread that is.
            Hide
            Parejkoj John Parejko added a comment -

            I worry that such solutions tie our code even closer to FITS. If we are going to do that, we might as well be explicit about it.

            Our continued use of HIERARCH in this manner also continues to worry me. It is supported by cfitsio, but its original design was not intended to be used as a "long keyword" convention, and cfitsio's use in this manner is one interpretation of the convention document: https://fits.gsfc.nasa.gov/registry/hierarch/hierarch.pdf

            For the record, here are a couple relevant Community posts where issues related to this have been discussed in the past:

            https://community.lsst.org/t/fits-and-lowercase-header-keys/1184/3
            https://community.lsst.org/t/safer-handling-of-long-fits-keywords/456/12

            Show
            Parejkoj John Parejko added a comment - I worry that such solutions tie our code even closer to FITS. If we are going to do that, we might as well be explicit about it. Our continued use of HIERARCH in this manner also continues to worry me. It is supported by cfitsio, but its original design was not intended to be used as a "long keyword" convention, and cfitsio's use in this manner is one interpretation of the convention document: https://fits.gsfc.nasa.gov/registry/hierarch/hierarch.pdf For the record, here are a couple relevant Community posts where issues related to this have been discussed in the past: https://community.lsst.org/t/fits-and-lowercase-header-keys/1184/3 https://community.lsst.org/t/safer-handling-of-long-fits-keywords/456/12
            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            Thanks for the link. That was not a side of PropertySet I'd had to deal with before.

            On the ExposureInfo side, the creation of header keywords based on component names was always a hack to keep compatibility with the old Exposure format (where the use of FITS is, explicitly, part of the Exposure API). Assuming that Jim Bosch still intends to switch over to a new persistence framework, we will likely have to change the FITS format anyway, and the hope was that we could then store component names in a more natural and less structural way. That's why I'm fine with requiring upper case for now.

            EDIT: Okay, we could implement the KEY_ half of KEY_/VAL_ in ExposureInfo right now, and keep round-tripping of lowercase component names. But that would make ExposureInfo's format even more elaborate, and I wouldn't necessarily trust the result.

            Show
            krzys Krzysztof Findeisen added a comment - - edited Thanks for the link. That was not a side of PropertySet I'd had to deal with before. On the ExposureInfo side, the creation of header keywords based on component names was always a hack to keep compatibility with the old Exposure format (where the use of FITS is, explicitly, part of the Exposure API). Assuming that Jim Bosch still intends to switch over to a new persistence framework, we will likely have to change the FITS format anyway, and the hope was that we could then store component names in a more natural and less structural way. That's why I'm fine with requiring upper case for now. EDIT: Okay, we could implement the KEY_ half of KEY_ / VAL_ in ExposureInfo right now, and keep round-tripping of lowercase component names. But that would make ExposureInfo 's format even more elaborate, and I wouldn't necessarily trust the result.
            Hide
            bvan Brian Van Klaveren added a comment -

            Mitigations have been performed on DM-21989 which unblock DM-21989.

            A new ticket, DM-24376, has been created for work which would warn when this issue might occur.

            Show
            bvan Brian Van Klaveren added a comment - Mitigations have been performed on DM-21989 which unblock DM-21989 . A new ticket, DM-24376 , has been created for work which would warn when this issue might occur.

              People

              • Assignee:
                Unassigned
                Reporter:
                bvan Brian Van Klaveren
                Watchers:
                Brian Van Klaveren, John Parejko, Krzysztof Findeisen, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel