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

polygon masking in CoaddPsf

    XMLWordPrintable

    Details

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

      Description

      We need to create polygon-based masks of the usable area of the focal plane, persist them with exposure, and include them in coaddition of PSFs and aperture corrections.

      This includes HSC issues HSC-972, HSC-973, HSC-974, HSC-975, HSC-976.

      At least some of this will be blocked by DM-833, which is the port issue for coaddition of aperture corrections.

        Attachments

          Issue Links

            Activity

            No builds found.
            jbosch Jim Bosch created issue -
            jbosch Jim Bosch made changes -
            Field Original Value New Value
            Epic Link DM-1942 [ 16008 ]
            jbosch Jim Bosch made changes -
            Link This issue FF-depends on DM-833 [ DM-833 ]
            jbosch Jim Bosch made changes -
            Labels HSC
            jbosch Jim Bosch made changes -
            Component/s afw [ 10714 ]
            Component/s meas_algorithms [ 10732 ]
            Hide
            tjenness Tim Jenness added a comment -

            How are you specifying the polygons? STC-S format would be handy.

            Show
            tjenness Tim Jenness added a comment - How are you specifying the polygons? STC-S format would be handy.
            Hide
            jbosch Jim Bosch added a comment -

            We don't actually have any text-based format for the polygons right now; they're created programmatically and persisted to FITS binary tables.

            Show
            jbosch Jim Bosch added a comment - We don't actually have any text-based format for the polygons right now; they're created programmatically and persisted to FITS binary tables.
            swinbank John Swinbank made changes -
            Assignee John Swinbank [ swinbank ]
            swinbank John Swinbank made changes -
            Assignee John Swinbank [ swinbank ]
            swinbank John Swinbank made changes -
            Sprint Science Pipelines DM-S15-5 [ 162 ]
            swinbank John Swinbank made changes -
            Sprint Science Pipelines DM-S15-5 [ 162 ]
            swinbank John Swinbank made changes -
            Assignee John Swinbank [ swinbank ] Russell Owen [ rowen ]
            Hide
            swinbank John Swinbank added a comment -

            Assigning this to Russell following discussion with Simon Krughoff on 26 June.

            Show
            swinbank John Swinbank added a comment - Assigning this to Russell following discussion with Simon Krughoff on 26 June.
            krughoff Simon Krughoff made changes -
            Link This issue is contained by DLP-287 [ DLP-287 ]
            krughoff Simon Krughoff made changes -
            Link This issue is contained by DLP-287 [ DLP-287 ]
            Hide
            rowen Russell Owen added a comment - - edited

            It looks like the first step is to pull over the use of Polygon in Exposure and ExposureInfo.

            Show
            rowen Russell Owen added a comment - - edited It looks like the first step is to pull over the use of Polygon in Exposure and ExposureInfo.
            Hide
            jbosch Jim Bosch added a comment -

            Yes, that's right. We already have an older version of the Polygon class on the LSST side, actually, but it's directly in afw::geom, and on the HSC side I found that I had to move it to a subpackage in order to avoid some Swig circular dependencies when adding persistence to it.

            Show
            jbosch Jim Bosch added a comment - Yes, that's right. We already have an older version of the Polygon class on the LSST side, actually, but it's directly in afw::geom, and on the HSC side I found that I had to move it to a subpackage in order to avoid some Swig circular dependencies when adding persistence to it.
            rowen Russell Owen made changes -
            Link This issue relates to DM-3198 [ DM-3198 ]
            rowen Russell Owen made changes -
            Link This issue is blocked by DM-3214 [ DM-3214 ]
            Hide
            rowen Russell Owen added a comment - - edited

            Paul: do you have time to look at this? The code itself will not need much review as it is cherry-picked from HSC. What needs review is any changes I made and my commits. All work is on tickets/DM-2981 of afw.

            I imagine it will be obvious, but the sequence of commits is as follows:

            • The first six commits are directly cherry-picked from HSC, followed by a few of mine that modernize the code. That set introduces the Polygon class.
            • The next cherry-pick adds validPolygon to ExposureInfo, followed by one trivial cleanup commit on my part.
            • The final commit fixes DM-3214, a bug that was exposed by an existing unit test but that was triggered by adding validPolygon to ExposureINfo. The fix was so trivial that I was given permission to fix it here. So I intend to take permission to merge this as permission to close DM-3214 as well.
            Show
            rowen Russell Owen added a comment - - edited Paul: do you have time to look at this? The code itself will not need much review as it is cherry-picked from HSC. What needs review is any changes I made and my commits. All work is on tickets/ DM-2981 of afw. I imagine it will be obvious, but the sequence of commits is as follows: The first six commits are directly cherry-picked from HSC, followed by a few of mine that modernize the code. That set introduces the Polygon class. The next cherry-pick adds validPolygon to ExposureInfo, followed by one trivial cleanup commit on my part. The final commit fixes DM-3214 , a bug that was exposed by an existing unit test but that was triggered by adding validPolygon to ExposureINfo. The fix was so trivial that I was given permission to fix it here. So I intend to take permission to merge this as permission to close DM-3214 as well.
            rowen Russell Owen made changes -
            Reviewers Paul Price [ price ]
            Status To Do [ 10001 ] In Review [ 10004 ]
            Hide
            price Paul Price added a comment -

            Looks good. I have just a few minor suggestions that you may or may not want to update:

            • When you have a conflict recorded in the commit message, it's helpful to explain what caused the conflicts and how they were resolved.
            • When you say "Modernize ...", could you explain in the commit message what has changed between ancient and modern times?
            • For "Modernize Polygon.cc's handling of Point fields in afw tables", the added code has trailing whitespace.
            • For "Fix ChebyshevBoundedField's...", your commit message is a bit wide on the last line.
            Show
            price Paul Price added a comment - Looks good. I have just a few minor suggestions that you may or may not want to update: When you have a conflict recorded in the commit message, it's helpful to explain what caused the conflicts and how they were resolved. When you say "Modernize ...", could you explain in the commit message what has changed between ancient and modern times? For "Modernize Polygon.cc's handling of Point fields in afw tables", the added code has trailing whitespace. For "Fix ChebyshevBoundedField's...", your commit message is a bit wide on the last line.
            price Paul Price made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            Hide
            rowen Russell Owen added a comment -

            Thank you, Paul. I did not remember what the merge conflicts were about, so I could not enhance those merge messages, but I clarified the other messages you mentioned and removed all trailing whitespace from several of the files that were part of this ticket. After a successful Jenkins build I merged to master.

            Show
            rowen Russell Owen added a comment - Thank you, Paul. I did not remember what the merge conflicts were about, so I could not enhance those merge messages, but I clarified the other messages you mentioned and removed all trailing whitespace from several of the files that were part of this ticket. After a successful Jenkins build I merged to master.
            rowen Russell Owen made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            swinbank John Swinbank made changes -
            Link This issue relates to DM-3243 [ DM-3243 ]
            Hide
            swinbank John Swinbank added a comment -

            Note that (contrary to the description) this did not pull over HSC-974. That will be done in DM-3243.

            Show
            swinbank John Swinbank added a comment - Note that (contrary to the description) this did not pull over HSC-974. That will be done in DM-3243 .
            Hide
            swinbank John Swinbank added a comment - - edited

            Pretty sure this doesn't have HSC-975 or HSC-976 either.

            Show
            swinbank John Swinbank added a comment - - edited Pretty sure this doesn't have HSC-975 or HSC-976 either.
            Hide
            swinbank John Swinbank added a comment -

            On further digging: HSC-975 doesn't apply until we have CoaddBoundedField (or equivalent), which is DM-833. I'll add a note to that issue to indicate that it's required.

            I think we can bring HSC-976 over straight away, though.

            Show
            swinbank John Swinbank added a comment - On further digging: HSC-975 doesn't apply until we have CoaddBoundedField (or equivalent), which is DM-833 . I'll add a note to that issue to indicate that it's required. I think we can bring HSC-976 over straight away, though.
            swinbank John Swinbank made changes -
            Link This issue relates to DM-3259 [ DM-3259 ]
            swinbank John Swinbank made changes -
            Labels HSC

              People

              Assignee:
              rowen Russell Owen
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              Paul Price
              Watchers:
              Jim Bosch, John Swinbank, Paul Price, Russell Owen, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.