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

Include polygon bounds in CoaddPsf logic

    Details

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

      Description

      This is a port of HSC-974. Original description:

      The CoaddPsf class should use the polygon bounding areas that were added to Exposure and ExposureRecord in DM-2981 (was: HSC-973) when determining which PSF images to coadd.

        Attachments

          Issue Links

            Activity

            Hide
            swinbank John Swinbank added a comment -

            Russell, would you mind taking a look? The work is on:

            It's mostly a straightforward cherry pick from HSC, but I did some tidying up to better comply with LSST standards.

            Show
            swinbank John Swinbank added a comment - Russell, would you mind taking a look? The work is on: tickets/DM-3243 on afw , and tickets/DM-3243 on meas_algorithms . It's mostly a straightforward cherry pick from HSC, but I did some tidying up to better comply with LSST standards.
            Hide
            rowen Russell Owen added a comment - - edited

            Review of afw changes:

            Overall this looks great.

            One request: in the descriptions of the various modified Exposure methods please document that includeValidPolygon is silently ignored if the Exposure has no valid polygon. One could imagine other ways of handling this, so it's best to be explicit.

            At risk of being too picky, please use curly braces in all control structures. Our coding standard do call for that (see 6-7 in https://confluence.lsstcorp.org/pages/viewpage.action?pageId=16908737#C++LayoutandComments-Layout) and I find the consistency really helps readability.

            Please file a ticket to add a unit test for this new functionality and link it to this ticket.

            Show
            rowen Russell Owen added a comment - - edited Review of afw changes: Overall this looks great. One request: in the descriptions of the various modified Exposure methods please document that includeValidPolygon is silently ignored if the Exposure has no valid polygon. One could imagine other ways of handling this, so it's best to be explicit. At risk of being too picky, please use curly braces in all control structures. Our coding standard do call for that (see 6-7 in https://confluence.lsstcorp.org/pages/viewpage.action?pageId=16908737#C++LayoutandComments-Layout ) and I find the consistency really helps readability. Please file a ticket to add a unit test for this new functionality and link it to this ticket.
            Hide
            rowen Russell Owen added a comment - - edited

            Review of meas_algorithms changes

            Another nice clean change, and the addition of a unit test is much appreciated!

            This is mostly in the category of cleaning up existing documentation, and thus possibly out of scope, but...

            • It would helpful if getValidPolygon was documented to throw pex::exceptions::RangeError if index was out of range, and likewise for all the other get methods that take an index.
            • It would be helpful if the doc strings were only in one place – preferably the .h file. Having them in two places can lead to one going stale (and not necessarily the one that will be picked up by Doxygen or read by users).
            Show
            rowen Russell Owen added a comment - - edited Review of meas_algorithms changes Another nice clean change, and the addition of a unit test is much appreciated! This is mostly in the category of cleaning up existing documentation, and thus possibly out of scope, but... It would helpful if getValidPolygon was documented to throw pex::exceptions::RangeError if index was out of range, and likewise for all the other get methods that take an index. It would be helpful if the doc strings were only in one place – preferably the .h file. Having them in two places can lead to one going stale (and not necessarily the one that will be picked up by Doxygen or read by users).
            Hide
            swinbank John Swinbank added a comment - - edited

            Thanks! Ticket for the missing tests is on DM-3349. Will merge to master shortly.

            Show
            swinbank John Swinbank added a comment - - edited Thanks! Ticket for the missing tests is on DM-3349 . Will merge to master shortly.

              People

              • Assignee:
                swinbank John Swinbank
                Reporter:
                swinbank John Swinbank
                Reviewers:
                Russell Owen
                Watchers:
                John Swinbank, Russell Owen
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel