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

Implement a visit-level BestSeeing selector in Gen3

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Labels:
    • Story Points:
      4
    • Sprint:
      DRP S21a (Dec Jan), DRP S21b
    • Team:
      Data Release Production
    • Urgent?:
      No

      Description

      To support Gen3 image differencing pipelines.

      DM-20695 implemented the ccd-level selectors run at makeWarp. Runs on a list of ccds for one single visit.

      This will implement the visit-level selectors done during coaddition. Operates on a list of deepCoadd_directWarps and other visit-level data products like sourceTable_visits

        Attachments

          Activity

          No builds found.
          yusra Yusra AlSayyad created issue -
          yusra Yusra AlSayyad made changes -
          Field Original Value New Value
          Status To Do [ 10001 ] In Progress [ 3 ]
          Hide
          yusra Yusra AlSayyad added a comment -

          Eric Bellm and Meredith Rawls Gen3 implementation of the your BestSeeingSelectVisitsTasK is ready for review. You're probably busy for the next 36 hours so it can wait until Thurs/Fri. If the class ends in "SelectImagesTask" it selects calexps for warping. If it ends in "SelectVisitsTask" (such as these new ones), it selects warps for coaddition. BestSeeingSelectVisitsTasks is the implementation of the existing gen2 BestSeeingSelectImages, just using catalogs as input. The pixelscale is slightly different than that computed from the per-ccd wcs. The PSF FWHM is spot on, however. I also included my own BestSeeingPercentileSelectVisitsTasks which we manually used this sprint and I plan on using as the default for DRP until we validate something better.

          Took Nate Lust's suggestion to write out the selected visit lists as output and have assemble read them in as input. He pointed out the StructuredDataList isn't working yet and suggested I use the storage class StructuredDataDict. (If you're wondering why I'm storing them as a dict instead of a list).

          Nate Lust, would you take a look at my pipeline file since I know you have a vision on how these should look:
          https://github.com/lsst/pipe_tasks/blob/865f4040131154d0034ded6e21f381f1d5cad218/pipelines/_DiffimDRP.yaml
          I didn't know how to specify the template

          {outputCoaddName}

          = bestSeeing so I just specified the name of the three output connections. Please advise.

          Show
          yusra Yusra AlSayyad added a comment - Eric Bellm and Meredith Rawls Gen3 implementation of the your BestSeeingSelectVisitsTasK is ready for review. You're probably busy for the next 36 hours so it can wait until Thurs/Fri. If the class ends in "SelectImagesTask" it selects calexps for warping. If it ends in "SelectVisitsTask" (such as these new ones), it selects warps for coaddition. BestSeeingSelectVisitsTasks is the implementation of the existing gen2 BestSeeingSelectImages, just using catalogs as input. The pixelscale is slightly different than that computed from the per-ccd wcs. The PSF FWHM is spot on, however. I also included my own BestSeeingPercentileSelectVisitsTasks which we manually used this sprint and I plan on using as the default for DRP until we validate something better. Took Nate Lust 's suggestion to write out the selected visit lists as output and have assemble read them in as input. He pointed out the StructuredDataList isn't working yet and suggested I use the storage class StructuredDataDict. (If you're wondering why I'm storing them as a dict instead of a list). Nate Lust , would you take a look at my pipeline file since I know you have a vision on how these should look: https://github.com/lsst/pipe_tasks/blob/865f4040131154d0034ded6e21f381f1d5cad218/pipelines/_DiffimDRP.yaml I didn't know how to specify the template {outputCoaddName} = bestSeeing so I just specified the name of the three output connections. Please advise.
          Show
          yusra Yusra AlSayyad added a comment - https://ci.lsst.codes/job/stack-os-matrix/33801/display/redirect
          yusra Yusra AlSayyad made changes -
          Labels drp-gen3
          yusra Yusra AlSayyad made changes -
          Labels drp-gen3
          yusra Yusra AlSayyad made changes -
          Labels drp-gen3
          Hide
          mrawls Meredith Rawls added a comment -

          I left comments on GitHub. Did you mean to put this ticket all the way in review, or are you still planning to do more things?

          Show
          mrawls Meredith Rawls added a comment - I left comments on GitHub. Did you mean to put this ticket all the way in review, or are you still planning to do more things?
          Hide
          yusra Yusra AlSayyad added a comment -

          Thanks for the review. I think I addressed all your comments, including your not liking the parquet inputs. I swapped them out for the new ExposureCatalog `visitSummary`. (This was needed anyway when I add MJD bounds next ticket). With DM-29407 on master from last week, it’s super fast. My test run with 33 visits now takes <4s.

          The idea was that BestSeeingSelectVisitTask, would be close to your original BestSeeingSelectImagesTask so that it’d look familiar and you could maintain along with the old Gen2 tasks. This was intended to be the task you maintained for AP, so I’m happy to make any edits you’d like to it. If you’re free to pair-code Tues/Wed, I’ll change all the variable names and default configs to whatever you want.
          BestSeeeingQuantileSelectVisitsTask is the new one I’m planning going to use for DRP.

          And now that visitSummary has the info, I also added a double check that the visits do indeed overlap the patch. This probably could be refined in the future to use getValidPolygon instead of the corners.

          And whoops, need to "Ask for Review"

          Show
          yusra Yusra AlSayyad added a comment - Thanks for the review. I think I addressed all your comments, including your not liking the parquet inputs. I swapped them out for the new ExposureCatalog `visitSummary`. (This was needed anyway when I add MJD bounds next ticket). With DM-29407 on master from last week, it’s super fast. My test run with 33 visits now takes <4s. The idea was that BestSeeingSelectVisitTask , would be close to your original BestSeeingSelectImagesTask so that it’d look familiar and you could maintain along with the old Gen2 tasks. This was intended to be the task you maintained for AP, so I’m happy to make any edits you’d like to it. If you’re free to pair-code Tues/Wed, I’ll change all the variable names and default configs to whatever you want. BestSeeeingQuantileSelectVisitsTask is the new one I’m planning going to use for DRP. And now that visitSummary has the info, I also added a double check that the visits do indeed overlap the patch. This probably could be refined in the future to use getValidPolygon instead of the corners. And whoops, need to "Ask for Review"
          yusra Yusra AlSayyad made changes -
          Reviewers Meredith Rawls [ mrawls ]
          Status In Progress [ 3 ] In Review [ 10004 ]
          Hide
          mrawls Meredith Rawls added a comment -

          I really like the new `visitSummary`, thanks for using it here, and thanks also for your thoughtful replies to my initial comments.

          The only thing these new tasks could use is some unit tests. If you'd like to get this functionality in the weekly for the release, I'm OK with a Jenkins run + merge today, and please file a ticket to add tests. If you don't think it's feasible to push it through today, please add tests on this ticket and I can take a final look when it's ready.

          Overall I'm very happy with this, and I hope Jenkins is too!

          Show
          mrawls Meredith Rawls added a comment - I really like the new `visitSummary`, thanks for using it here, and thanks also for your thoughtful replies to my initial comments. The only thing these new tasks could use is some unit tests. If you'd like to get this functionality in the weekly for the release, I'm OK with a Jenkins run + merge today, and please file a ticket to add tests. If you don't think it's feasible to push it through today, please add tests on this ticket and I can take a final look when it's ready. Overall I'm very happy with this, and I hope Jenkins is too!
          Hide
          yusra Yusra AlSayyad added a comment -

          Ticket filed for unit tests: DM-29894.
          DM-29311 almost ready for review. Want to continuing being default reviewer for the visit selectors or should I spread it around?

          Show
          yusra Yusra AlSayyad added a comment - Ticket filed for unit tests: DM-29894 . DM-29311 almost ready for review. Want to continuing being default reviewer for the visit selectors or should I spread it around?
          yusra Yusra AlSayyad made changes -
          Sprint DRP S21a (Dec Jan) [ 1071 ] DRP S21a (Dec Jan), DRP S21b [ 1071, 1094 ]
          yusra Yusra AlSayyad made changes -
          Epic Link DM-27974 [ 442759 ]
          yusra Yusra AlSayyad made changes -
          Status In Review [ 10004 ] Reviewed [ 10101 ]
          yusra Yusra AlSayyad made changes -
          Resolution Done [ 10000 ]
          Status Reviewed [ 10101 ] Done [ 10002 ]
          Hide
          mrawls Meredith Rawls added a comment -

          I nominate alternating between myself and Eric Bellm; I could do this next one eventually if his backlog is overflowing more than mine.

          Show
          mrawls Meredith Rawls added a comment - I nominate alternating between myself and Eric Bellm ; I could do this next one eventually if his backlog is overflowing more than mine.
          yusra Yusra AlSayyad made changes -
          Epic Link DM-27974 [ 442759 ] DM-29152 [ 458509 ]
          yusra Yusra AlSayyad made changes -
          Story Points 4

            People

            Assignee:
            yusra Yusra AlSayyad
            Reporter:
            yusra Yusra AlSayyad
            Reviewers:
            Meredith Rawls
            Watchers:
            Meredith Rawls, Yusra AlSayyad
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Jenkins

                No builds found.