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

Update existing cp_pipe tasks to pipelineTasks

    XMLWordPrintable

    Details

    • Story Points:
      40
    • Sprint:
      DRP F19-6 (Nov)
    • Team:
      Data Release Production

      Description

      The new tasks for cp_pipe will need to be ready to work with the gen3 butler/as pipelineTasks in the near future, which likely requires some sort of plan.  This ticket is to remind everyone of that fact.

        Attachments

          Issue Links

            Activity

            czw Christopher Waters created issue -
            czw Christopher Waters made changes -
            Field Original Value New Value
            Link This issue FF-depends on DM-20069 [ DM-20069 ]
            Hide
            mfisherlevine Merlin Fisher-Levine added a comment -

            Thanks for making this ticket Chris. Just let me know when you'd like to have a discussion.

            Show
            mfisherlevine Merlin Fisher-Levine added a comment - Thanks for making this ticket Chris. Just let me know when you'd like to have a discussion.
            czw Christopher Waters made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            czw Christopher Waters made changes -
            Assignee Christopher Waters [ cwaters ]
            plazas Andrés Alejandro Plazas Malagón made changes -
            Link This issue is parent task of DM-21786 [ DM-21786 ]
            jbosch Jim Bosch made changes -
            Labels gen3-middleware
            jbosch Jim Bosch made changes -
            Labels gen3-middleware gen2-deprecation-blocker gen3-middleware
            Hide
            czw Christopher Waters added a comment -

            I've just sent this into review. Hopefully the github review process has pointed Nate Lust and Paul Price to the appropriate PRs to look at, but generally, I would like Nate to look over the gen3/pipelineTask implementation, and Paul to review the calibration construction process.

            To assist in this process, I have constructed calibrations with both the gen3 and gen2 codebases, with output results in

            {'gen2': '/project/czw/DM-21212/Tgen2.20191015', 'gen3': '/project/czw/gen3_hsc/DM-21212/'}

            . Image differences confirm that there are only small offsets between the generations (~1e-4).

            There are somewhat higher residuals in the SKY calibration for reasons that are not currently clear. However, I would like to merge this code with that potential caveat (and file a future ticket to resolve that) to prevent the other cp_pipe development from diverging.

            Show
            czw Christopher Waters added a comment - I've just sent this into review. Hopefully the github review process has pointed Nate Lust and Paul Price to the appropriate PRs to look at, but generally, I would like Nate to look over the gen3/pipelineTask implementation, and Paul to review the calibration construction process. To assist in this process, I have constructed calibrations with both the gen3 and gen2 codebases, with output results in {'gen2': '/project/czw/DM-21212/Tgen2.20191015', 'gen3': '/project/czw/gen3_hsc/DM-21212/'} . Image differences confirm that there are only small offsets between the generations (~1e-4). There are somewhat higher residuals in the SKY calibration for reasons that are not currently clear. However, I would like to merge this code with that potential caveat (and file a future ticket to resolve that) to prevent the other cp_pipe development from diverging.
            czw Christopher Waters made changes -
            Reviewers Nate Lust, Paul Price [ nlust, price ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            Hide
            tjenness Tim Jenness added a comment -

            Since this is a port of constructCalibs.py from pipe_drivers, have you compared the output headers between this new version and the pipe_drivers outputs? pipe_drivers has code for propagating input headers to outputs using astro_metadata_translator but I don't see any astro_metadata_translator calls in cp_pipe so I'm wondering how the merge happens now.

            Show
            tjenness Tim Jenness added a comment - Since this is a port of constructCalibs.py from pipe_drivers, have you compared the output headers between this new version and the pipe_drivers outputs? pipe_drivers has code for propagating input headers to outputs using astro_metadata_translator but I don't see any astro_metadata_translator calls in cp_pipe so I'm wondering how the merge happens now.
            Hide
            price Paul Price added a comment -

            I don't believe this implements the same workflow as the original flat and sky construction from pipe_drivers (the scatter/gather is missing in both cases), which means that the outputs are incorrect. I suspect this is going to involve substantial additional work.

            Show
            price Paul Price added a comment - I don't believe this implements the same workflow as the original flat and sky construction from pipe_drivers (the scatter/gather is missing in both cases), which means that the outputs are incorrect. I suspect this is going to involve substantial additional work.
            nlust Nate Lust made changes -
            Watchers Andrés Alejandro Plazas Malagón, Christopher Waters, Merlin Fisher-Levine, Nate Lust, Paul Price, Tim Jenness [ Andrés Alejandro Plazas Malagón, Christopher Waters, Merlin Fisher-Levine, Nate Lust, Paul Price, Tim Jenness ] Andrés Alejandro Plazas Malagón, Christopher Waters, Merlin Fisher-Levine, Paul Price, Tim Jenness [ Andrés Alejandro Plazas Malagón, Christopher Waters, Merlin Fisher-Levine, Paul Price, Tim Jenness ]
            czw Christopher Waters made changes -
            Link This issue is FF-depended by DM-22534 [ DM-22534 ]
            czw Christopher Waters made changes -
            Link This issue is FF-depended by DM-22527 [ DM-22527 ]
            czw Christopher Waters made changes -
            Link This issue is FF-depended by DM-22521 [ DM-22521 ]
            czw Christopher Waters made changes -
            Link This issue is FF-depended by DM-22302 [ DM-22302 ]
            yusra Yusra AlSayyad made changes -
            Epic Link DM-21252 [ 414682 ]
            yusra Yusra AlSayyad made changes -
            Sprint DRP F19-6 (Nov) [ 972 ]
            yusra Yusra AlSayyad made changes -
            Epic Link DM-21252 [ 414682 ] DM-21254 [ 414685 ]
            czw Christopher Waters made changes -
            Epic Link DM-21254 [ 414685 ] DM-22586 [ 427653 ]
            Hide
            czw Christopher Waters added a comment -

            Sorry for the long delay, but I would like to ask for a rereview of this ticket.  The flat code now normalizes the inputs by gain to get the incident flux (in electrons), and use this value to normalize exposures to a consistent input level.  The remaining changes are those needed to keep up with the pace of gen3 development.

            Show
            czw Christopher Waters added a comment - Sorry for the long delay, but I would like to ask for a rereview of this ticket.  The flat code now normalizes the inputs by gain to get the incident flux (in electrons), and use this value to normalize exposures to a consistent input level.  The remaining changes are those needed to keep up with the pace of gen3 development.
            Hide
            tjenness Tim Jenness added a comment -

            Are we entirely clear which parts of cp_pipe work on exposures and which work on visits? Gen3 is about to make them different so it would be good to know ahead of time whether this means that all occurrence of visit changes to exposure_id or if some stay as visits. (we can't make the change in this ticket).

            Show
            tjenness Tim Jenness added a comment - Are we entirely clear which parts of cp_pipe work on exposures and which work on visits? Gen3 is about to make them different so it would be good to know ahead of time whether this means that all occurrence of visit changes to exposure_id or if some stay as visits. (we can't make the change in this ticket).
            tjenness Tim Jenness made changes -
            Link This issue blocks DM-23576 [ DM-23576 ]
            Hide
            mfisherlevine Merlin Fisher-Levine added a comment -

            I can help discuss this after the AuxTel run if necessary, but I'm sure Chris doesn't really need me for that.

            Show
            mfisherlevine Merlin Fisher-Levine added a comment - I can help discuss this after the AuxTel run if necessary, but I'm sure Chris doesn't really need me for that.
            yusra Yusra AlSayyad made changes -
            Epic Link DM-22586 [ 427653 ] DM-23737 [ 431393 ]
            Hide
            czw Christopher Waters added a comment -

            I believe I have sorted out the flat issue, and will be getting this back to active review once I have the code cleaned/commits rebased/documentation updated/etc.

            A notebook containing the validation of the new gen3 flats and a quick discussion of changes is available here: https://github.com/czwa/notebooks/blob/master/20200527-lsst_gen3_flat_scaling/flatscale.20200527.ipynb

            Show
            czw Christopher Waters added a comment - I believe I have sorted out the flat issue, and will be getting this back to active review once I have the code cleaned/commits rebased/documentation updated/etc. A notebook containing the validation of the new gen3 flats and a quick discussion of changes is available here:  https://github.com/czwa/notebooks/blob/master/20200527-lsst_gen3_flat_scaling/flatscale.20200527.ipynb
            Hide
            czw Christopher Waters added a comment -

            I would like to re-request review on this ticket.  The algorithm now matches the gen2 implementation scaling between detectors.  The code should now be better documented and clearer to follow than it was before.

            Show
            czw Christopher Waters added a comment - I would like to re-request review on this ticket.  The algorithm now matches the gen2 implementation scaling between detectors.  The code should now be better documented and clearer to follow than it was before.
            Hide
            price Paul Price added a comment -

            I think the only comments I have are minor, so go ahead and merge after cleaning up.

            The commit history is not orthogonal, and I would urge you to put some effort into cleaning this up before merging. Squash commits dealing with the same new code, and split commits that contain multiple functional changes. It might be easiest to simply toss all your commits and start crafting new ones from your changes. Let me know if you need help on doing any of that.

            Show
            price Paul Price added a comment - I think the only comments I have are minor, so go ahead and merge after cleaning up. The commit history is not orthogonal, and I would urge you to put some effort into cleaning this up before merging. Squash commits dealing with the same new code, and split commits that contain multiple functional changes. It might be easiest to simply toss all your commits and start crafting new ones from your changes. Let me know if you need help on doing any of that.
            price Paul Price made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            yusra Yusra AlSayyad made changes -
            Epic Link DM-23737 [ 431393 ] DM-25321 [ 435711 ]
            Show
            czw Christopher Waters added a comment - https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/32080/pipeline/
            czw Christopher Waters made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            czw Christopher Waters made changes -
            Story Points 40
            yusra Yusra AlSayyad made changes -
            Epic Link DM-25321 [ 435711 ] DM-23740 [ 431397 ]

              People

              Assignee:
              czw Christopher Waters
              Reporter:
              czw Christopher Waters
              Reviewers:
              Nate Lust, Paul Price
              Watchers:
              Andrés Alejandro Plazas Malagón, Christopher Waters, Merlin Fisher-Levine, Paul Price, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: