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

Use FilterLabel in Exposure/ExposureInfo

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Labels:
    • Story Points:
      10
    • Sprint:
      AP F20-6 (November), AP S21-1 (December)
    • Team:
      Alert Production
    • Urgent?:
      No

      Description

      Once FilterLabel is implemented, add it to a new persistence format for ExposureInfo. Ensure that Exposure has methods that work in both Filter and FilterLabel, and that older, Filter-based files are converted in a reasonable manner. This is a large ticket, as it requires that backwards-compatibility code be provided at both the class and persistence levels.

      I recommend that DM-27168 be done before this ticket, so that any discoveries about unexpected use cases can be incorporated into FilterLabel early.

        Attachments

          Issue Links

            Activity

            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            Discussion on DM-27167 concluded that the only aliases being used in the current Stack are the afw_name fields (HSC/r2, HSC/i2, DECam/SOLID), which are used as the canonical name for old-style filters and therefore written in exposures and catalogs. Unfortunately, we can't remove use of these names in a self-consistent fashion without breaking some code, somewhere, that depends on the current hybrid system.

            John Parejko suggested simply adding special-case code that detects these three names on reading old Exposure files (in addition to the band vs. physical check discussed on RFC-730). There is also some special-case code surrounding these names (https://github.com/lsst/obs_subaru/blob/master/python/lsst/obs/subaru/filterFraction.py) which would need to be modified to support physical filter names in addition to afw_names.

            Most of the Stack code assumes that Filter::getName returns a band, so the new Exposure::getFilter should prefer the band as the name passed to the Filter constructor. The exceptions, which would need to be rewritten before DM-27170, are:

            However, to make sure we haven't overlooked any cases, we should probably post a warning on CLO before merging.

            Show
            krzys Krzysztof Findeisen added a comment - - edited Discussion on DM-27167 concluded that the only aliases being used in the current Stack are the afw_name fields (HSC/r2, HSC/i2, DECam/SOLID), which are used as the canonical name for old-style filters and therefore written in exposures and catalogs. Unfortunately, we can't remove use of these names in a self-consistent fashion without breaking some code, somewhere, that depends on the current hybrid system. John Parejko suggested simply adding special-case code that detects these three names on reading old Exposure files (in addition to the band vs. physical check discussed on RFC-730 ). There is also some special-case code surrounding these names ( https://github.com/lsst/obs_subaru/blob/master/python/lsst/obs/subaru/filterFraction.py ) which would need to be modified to support physical filter names in addition to afw_names. Most of the Stack code assumes that Filter::getName returns a band, so the new Exposure::getFilter should prefer the band as the name passed to the Filter constructor. The exceptions, which would need to be rewritten before DM-27170 , are: https://github.com/lsst/pipe_tasks/blob/master/python/lsst/pipe/tasks/coaddInputRecorder.py#L115 However, to make sure we haven't overlooked any cases, we should probably post a warning on CLO before merging.
            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            N.B. Exposure currently persists its filters as Filter::getName(), not getCanonicalName(), so old files could use any name.

            Show
            krzys Krzysztof Findeisen added a comment - - edited N.B. Exposure currently persists its filters as Filter::getName() , not getCanonicalName() , so old files could use any name.
            Hide
            krzys Krzysztof Findeisen added a comment -

            Discussion on DM-27167 suggests that ExposureInfo should also be responsible for fixing old filter names in CoaddInputs. Not clear how practical this is, given that the schema for CoaddInputs is defined in pipe_tasks, not afw.

            Show
            krzys Krzysztof Findeisen added a comment - Discussion on DM-27167 suggests that ExposureInfo should also be responsible for fixing old filter names in CoaddInputs . Not clear how practical this is, given that the schema for CoaddInputs is defined in pipe_tasks , not afw .
            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            At the moment, the compatibility code does introduce one side effect: exposure.getFilter().getName() always returns the first defined of afw name, band, or physical filter. This is a behavior change for raw files, where getName() previously returned the physical filter. John Parejko has recommended allowing the behavior change, and migrating any code that specifically needs physical filter now instead of on DM-27170.

            This change does not affect any code that uses getCanonicalName(), or (AFAIK) any code operating on calexps.

            Show
            krzys Krzysztof Findeisen added a comment - - edited At the moment, the compatibility code does introduce one side effect: exposure.getFilter().getName() always returns the first defined of afw name, band, or physical filter. This is a behavior change for raw files, where getName() previously returned the physical filter. John Parejko has recommended allowing the behavior change, and migrating any code that specifically needs physical filter now instead of on DM-27170 . This change does not affect any code that uses getCanonicalName() , or (AFAIK) any code operating on calexps.
            Hide
            krzys Krzysztof Findeisen added a comment -

            John Parejko, could you review the changes in afw? It's 350 lines.

            Tim Jenness, could you review the changes in daf_butler and obs_base? It's 31 lines total.

            Show
            krzys Krzysztof Findeisen added a comment - John Parejko , could you review the changes in afw ? It's 350 lines. Tim Jenness , could you review the changes in daf_butler and obs_base ? It's 31 lines total.
            Hide
            krzys Krzysztof Findeisen added a comment -

            Sorry, spoke too soon. I found a problem that would require some significant rework of the Gen 3-related changes. I'll put this ticket back in review once I'm really sure it's ready.

            Show
            krzys Krzysztof Findeisen added a comment - Sorry, spoke too soon. I found a problem that would require some significant rework of the Gen 3-related changes. I'll put this ticket back in review once I'm really sure it's ready.
            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            Ok, everything builds on lsst_distrib, lsst_ci, ci_hsc, and ap_verify without further hacks, and a manual inspection by a non-rushed developer finds no compatibility problems other than the aforementioned bias away from physical filters.

            Now for real, John Parejko, can you review afw, and Tim Jenness, can you review daf_butler and obs_base? The latter has increased to 43 lines total.

            Show
            krzys Krzysztof Findeisen added a comment - - edited Ok, everything builds on lsst_distrib , lsst_ci , ci_hsc , and ap_verify without further hacks, and a manual inspection by a non-rushed developer finds no compatibility problems other than the aforementioned bias away from physical filters. Now for real, John Parejko , can you review afw , and Tim Jenness , can you review daf_butler and obs_base ? The latter has increased to 43 lines total.
            Hide
            Parejkoj John Parejko added a comment -

            I made a few comments on the afw PR (and obs_base because apparently I can't read...), but overall this looks quite good.

            Show
            Parejkoj John Parejko added a comment - I made a few comments on the afw PR (and obs_base because apparently I can't read...), but overall this looks quite good.

              People

              Assignee:
              krzys Krzysztof Findeisen
              Reporter:
              krzys Krzysztof Findeisen
              Reviewers:
              Tim Jenness
              Watchers:
              John Parejko, Krzysztof Findeisen, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  CI Builds

                  No builds found.