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

Enhance PTC task to use Pierre^2's code

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: cp_pipe
    • Labels:
      None

      Description

      Following on from DM-20069, make the PTC task work well be adding algorithmic code provided by Pierre^2.

        Attachments

          Issue Links

            Activity

            Hide
            rhl Robert Lupton added a comment -

            In #dm-drp you say, "This ticket will implement the covariances calculation in ptc.py of cp_pipe on images where ISR has been performed".  There are various different effects discussed in that paper (e.g. the effects of clipping, problems in the serials) in addition to an FFT-based approach.  Which of these are important for this ticket?

            Show
            rhl Robert Lupton added a comment - In #dm-drp you say, "This ticket will implement the covariances calculation in  ptc.py  of  cp_pipe  on images where ISR has been performed".  There are various different effects discussed in that paper (e.g. the effects of clipping, problems in the serials) in addition to an FFT-based approach.  Which of these are important for this ticket?
            Hide
            plazas Andrés Alejandro Plazas Malagón added a comment -

            That's right, thanks for your comment. There are several things discussed in that paper, and the general description of this ticket is vague. I think that in this ticket I will concentrate first on implementing the FFT-based approach of calculating the general covariances and producing the associated plots (i.e., what's in the appendix of the paper). They mention the effects on clipping, as you point out, which in their code (and in the paper), they address by including an extra factor in the covariances: https://github.com/PierreAstier/bfptc/blob/master/py/ptcfit.py#L91 (although if you look at the docstring, Pierre Astier writes that this factor should be actually included somewhere else). So this factor would be included too.

            There is also work on compensating for the deferred charge in the serial direction ("CTI") and to correct for non-linearity; I was thinking that, in our case, this would be covered in the corresponding ISR step, and hence in other potential new tickets.

            Do you agree?

            Show
            plazas Andrés Alejandro Plazas Malagón added a comment - That's right, thanks for your comment. There are several things discussed in that paper, and the general description of this ticket is vague. I think that in this ticket I will concentrate first on implementing the FFT-based approach of calculating the general covariances and producing the associated plots (i.e., what's in the appendix of the paper). They mention the effects on clipping, as you point out, which in their code (and in the paper), they address by including an extra factor in the covariances: https://github.com/PierreAstier/bfptc/blob/master/py/ptcfit.py#L91 (although if you look at the docstring, Pierre Astier writes that this factor should be actually included somewhere else). So this factor would be included too. There is also work on compensating for the deferred charge in the serial direction ("CTI") and to correct for non-linearity; I was thinking that, in our case, this would be covered in the corresponding ISR step, and hence in other potential new tickets. Do you agree?
            Hide
            rhl Robert Lupton added a comment -

            Sounds good.  I liked the approach of moving the other effects that they discuss (such as CTI) into the ISR – non-linearity should be there already

            Show
            rhl Robert Lupton added a comment - Sounds good.  I liked the approach of moving the other effects that they discuss (such as CTI) into the ISR – non-linearity should be there already
            Hide
            plazas Andrés Alejandro Plazas Malagón added a comment - - edited

            Most of the code for this ticket was ported/adapted from

            https://github.com/PierreAstier/bfptc/
            

            which was used to implement the models in Astier+19 (1905.08677, "The Shape of the Photon Transfer Curve in CCDs") to fit for the covariances from flat fields, in particular, the full model of Equation 20 (ptc.py already implemented the approximation in Equation 14 of that paper, to fit the model of that equation to the PTC curve (Cov00 (var) vs signal)). Pierre Astier kindly provided some slides to explain the structure of the code above: pipeline_astier_shape_of_ptc.pdf

            For this ticket, if doCovariancesAstier=True an array with tags (numpy.recarray, referred to as "tuple" in Pierre's code) is calculated and returned by computeCovariancesAstier from the input flat pairs. The array contains the covariances at all signal levels and all lags up to a maximum lag/range. Then, the function fitData uses this array to fit the data to the full model:

            The function plotCovariancesAstier reproduces some of the plots in Astier+19 (the style of a few of them changed to match the plotting style already in ptc.py). The plotting routines are in the file astierCovPtcPlots.py, fitData, in astierCovPtcFit.py, and the file astierCovFitParameters.py contains external code that aids in the fitting process (even external to Pierre, as evidenced by the disclaimer at the top of that file, which I chose to leave so that we can decide what to do with this external package). The file astierCovPtcUtils.py has some helper routines.

            I implemented some unit tests in test/test_ptc.py under test_covAstier. The tests check that the gain is the same as the input gain from mock data, that the covariances via FFT (as it is in MeasurePhotonTransferCurveTask when doCovariancesAstier=True) are the same as calculated in real space, and that Cov[0, 0] (i.e., the variances) are consistent with the variances calculated with the standard method (when doCovariancesAstier=false).

            I created some mock data to run the code (adding some brighter-fatter via the power-law model in GalSim); the resulting plots are here (the input gain of 0.75 e/ADU is recovered; same data for all channels): PTC_COVS_ASTIER_det0.pdf

            There are some other things that still could be done (e.g., testing the code with on-sky or lab data, refactoring the plotting code of ptc.py and astierCovPtcPlots.py to put them in the same place, as well as the code in utils.py and astierCovPtcUtils.py, implementing the correction for the bias induced in the variance due to sigma-clipping (discussed above and mentioned in the paper in section 5.2, but we have decided that we will do this later: DM-24762), etc), but with John Swinbank we decided that the code should be reviewed at the current state (as it is already a lot of code) and that further work should be tackled in other tickets.

            Show
            plazas Andrés Alejandro Plazas Malagón added a comment - - edited Most of the code for this ticket was ported/adapted from https://github.com/PierreAstier/bfptc/ which was used to implement the models in Astier+19 ( 1905.08677 , "The Shape of the Photon Transfer Curve in CCDs") to fit for the covariances from flat fields, in particular, the full model of Equation 20 ( ptc.py already implemented the approximation in Equation 14 of that paper, to fit the model of that equation to the PTC curve (Cov00 (var) vs signal)). Pierre Astier kindly provided some slides to explain the structure of the code above: pipeline_astier_shape_of_ptc.pdf For this ticket, if doCovariancesAstier=True an array with tags (numpy.recarray, referred to as "tuple" in Pierre's code) is calculated and returned by computeCovariancesAstier from the input flat pairs. The array contains the covariances at all signal levels and all lags up to a maximum lag/range. Then, the function fitData uses this array to fit the data to the full model: The function plotCovariancesAstier reproduces some of the plots in Astier+19 (the style of a few of them changed to match the plotting style already in ptc.py ). The plotting routines are in the file astierCovPtcPlots.py , fitData , in astierCovPtcFit.py , and the file astierCovFitParameters.py contains external code that aids in the fitting process (even external to Pierre, as evidenced by the disclaimer at the top of that file, which I chose to leave so that we can decide what to do with this external package). The file astierCovPtcUtils.py has some helper routines. I implemented some unit tests in test/test_ptc.py under test_covAstier . The tests check that the gain is the same as the input gain from mock data, that the covariances via FFT (as it is in MeasurePhotonTransferCurveTask when doCovariancesAstier=True ) are the same as calculated in real space, and that Cov [0, 0] (i.e., the variances) are consistent with the variances calculated with the standard method (when doCovariancesAstier=false). I created some mock data to run the code (adding some brighter-fatter via the power-law model in GalSim); the resulting plots are here (the input gain of 0.75 e/ADU is recovered; same data for all channels): PTC_COVS_ASTIER_det0.pdf There are some other things that still could be done (e.g., testing the code with on-sky or lab data, refactoring the plotting code of ptc.py and astierCovPtcPlots.py to put them in the same place, as well as the code in utils.py and astierCovPtcUtils.py , implementing the correction for the bias induced in the variance due to sigma-clipping (discussed above and mentioned in the paper in section 5.2, but we have decided that we will do this later: DM-24762 ), etc), but with John Swinbank we decided that the code should be reviewed at the current state (as it is already a lot of code) and that further work should be tackled in other tickets.
            Show
            plazas Andrés Alejandro Plazas Malagón added a comment - Latest Jenkins that passed: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/32075/pipeline
            Hide
            czw Christopher Waters added a comment -

            I have a lot of comments on the PR.  I'm worried that the current structure of MeasurePhotonTransferCurveTask makes it hard to change without adding long chains of independent code that are switched on and off from the configuration.  All of these configuration options should be returning the same set of things, so my hope is that we can move all the differences into a small number of measurement methods that are called by a run() method.

            This will also help make the move to gen3 easier than it appears to be at the moment.

            Show
            czw Christopher Waters added a comment - I have a lot of comments on the PR.  I'm worried that the current structure of MeasurePhotonTransferCurveTask makes it hard to change without adding long chains of independent code that are switched on and off from the configuration.  All of these configuration options should be returning the same set of things, so my hope is that we can move all the differences into a small number of measurement methods that are called by a run() method. This will also help make the move to gen3 easier than it appears to be at the moment.
            Hide
            czw Christopher Waters added a comment -

            A few missing docstrings, I think entirely in the plotPtc.py code.  I think as long as this builds cleanly on Jenkins, it's fine for now.  It'd be good to try to streamline this code in a future ticket, and maybe that'll be part of the gen3 conversion.

            Show
            czw Christopher Waters added a comment - A few missing docstrings, I think entirely in the plotPtc.py code.  I think as long as this builds cleanly on Jenkins, it's fine for now.  It'd be good to try to streamline this code in a future ticket, and maybe that'll be part of the gen3 conversion.
            Show
            plazas Andrés Alejandro Plazas Malagón added a comment - https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/32254/pipeline/

              People

              • Assignee:
                plazas Andrés Alejandro Plazas Malagón
                Reporter:
                mfisherlevine Merlin Fisher-Levine
                Reviewers:
                Christopher Waters
                Watchers:
                Andrés Alejandro Plazas Malagón, Christopher Waters, Merlin Fisher-Levine, Robert Lupton
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel