Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-24703: Make linearity a subclass of lsst.ip.isr.IsrCalib #48

Merged
merged 2 commits into from Sep 9, 2020

Conversation

czwa
Copy link
Contributor

@czwa czwa commented Aug 26, 2020

Move linearity calibration construction out of ptc.py, and make the new task gen3 compliant.

@czwa czwa requested a review from plazas August 26, 2020 23:59
Comment on lines +70 to +73
"LookupTable": "Create a lookup table solution.",
"Polynomial": "Create an arbitrary polynomial solution.",
"Squared": "Create a single order squared solution.",
"None": "Create a dummy solution.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Craig has recently been obtaining results that indicate that we might also need to implement a spline fit option. Could this be something added to this ticket, or perhaps something for another ticket?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another ticket please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added DM-26545 for this.

)

def debugFit(self, stepname, timeVector, meanVector, linearizer, ampName):
"""
Copy link
Contributor

@plazas plazas Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No docstring here? I guess up in L197 you are calling this function to produce linearity plots for debugging while the code is running, right? (like in the crosstalk code)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the plan. Since this can be run independent of the previous steps (ISR, PTC), the results can be checked through the debug code. I've added the missing docstring.

@@ -85,27 +84,17 @@ class MeasurePhotonTransferCurveTaskConfig(pexConfig.Config):
doc="Degree of polynomial to fit the PTC, when 'ptcFitType'=POLYNOMIAL.",
default=3,
)
linearity = pexConfig.ConfigurableField(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that this is used below when calling this new linearity subtask, but I don't quite understand why this is a configuration field. (why is it here at this point?). I'm probably missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By making this a ConfigurableField, the target task can be instantiated as a subtask (L334). This allows the PTC code to call the subtask's run method directly (L448). This moves the linearity configuration to config.linearity.polynomialOrderand similar fields, instead of being directly part of the MeasurePhotonTransferCurveTaskConfig.

Comment on lines 170 to 183
# @dataclass
# class LinearityResidualsAndLinearizersDataset:
# """A simple class to hold the output from the
# `calculateLinearityResidualAndLinearizers` function.
# """
# # Normalized coefficients for polynomial NL correction
# polynomialLinearizerCoefficients: list
# # Normalized coefficient for quadratic polynomial NL correction (c0)
# quadraticPolynomialLinearizerCoefficient: float
# # LUT array row for the amplifier at hand
# linearizerTableRow: list
# meanSignalVsTimePolyFitPars: list
# meanSignalVsTimePolyFitParsErr: list
# meanSignalVsTimePolyFitReducedChiSq: float
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that this dataclass is fully commented. If it is not being used anymore, then it should be completely removed.

Comment on lines -910 to -922
Coefficients for LinearizePolynomial, where corrImage = uncorrImage + sum_i c_i uncorrImage^(2 +
i).
c_(j-2) = -k_j/(k_1^j) with units DN^(1-j) (c.f., Eq. 37 of 2003.05978). The units of k_j are
DN/t^j, and they are fit from meanSignalVector = k0 + k1*exposureTimeVector +
k2*exposureTimeVector^2 + ... + kn*exposureTimeVector^n, with
n = "polynomialFitDegreeNonLinearity". k_0 and k_1 and degenerate with bias level and gain,
and are not used by the non-linearity correction. Therefore, j = 2...n in the above expression
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that this explanation and reference got lost, but I think it helped the user understand a bit better how the polynomial correction was constructed. Could this (or a summary) be placed back somewhere? Or maybe we could still document this in the comments of the Jira ticket, at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added and updated this discussion in the Notes section of the run method docstring.

@czwa czwa force-pushed the tickets/DM-24703 branch 2 times, most recently from 5f6b48c to 77c9443 Compare August 31, 2020 21:55
PTC now calls the linearity code as a subtask.
This updates the linearizer to be an IsrCalib.
Adds gen3 handling for linearity.

Use configurable test accuracy for linearity.
@czwa czwa merged commit 847fb6c into master Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants