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

Include polygon bounds in CoaddPsf logic

    XMLWordPrintable

    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

            No builds found.
            swinbank John Swinbank created issue -
            swinbank John Swinbank made changes -
            Field Original Value New Value
            Epic Link DM-1942 [ 16008 ]
            swinbank John Swinbank made changes -
            Link This issue relates to DM-2981 [ DM-2981 ]
            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.
            swinbank John Swinbank made changes -
            Reviewers Russell Owen [ rowen ]
            Status To Do [ 10001 ] In Review [ 10004 ]
            swinbank John Swinbank made changes -
            Link This issue blocks DM-3258 [ DM-3258 ]
            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.
            rowen Russell Owen made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            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).
            rowen Russell Owen made changes -
            Status Reviewed [ 10101 ] In Review [ 10004 ]
            rowen Russell Owen made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            swinbank John Swinbank made changes -
            Link This issue relates to DM-3349 [ DM-3349 ]
            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.
            swinbank John Swinbank made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]

              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:

                  Jenkins

                  No builds found.