DM-42766: Update linearity fitting code to fit temperature dependence and other updates recommended by Pierre Astier #239

erykoff · 2024-04-18T15:17:12Z

No description provided.

czwa

Mostly docstring comments.

czwa · 2024-04-22T21:25:19Z

python/lsst/cp/pipe/linearity.py

+                    raise ValueError("Config requests fitting temperature coefficient for "
+                                     f"{self.config.splineFitTemperatureColumn} but this column "
+                                     "is not available in inputPtc.auxValues.")
+                tempValue = inputPtc.auxValues[self.config.splineFitTemperatureColumn]


I think I'm going to suggest a different variable name for this. Am I correct that this is a vector of temperatures?

czwa · 2024-04-22T21:34:58Z

python/lsst/cp/pipe/linearity.py

-                # here, for proper residual computation.
+                # We must modify the inputOrdinate according to the
+                # nuisance terms in the linearity fit for the residual
+                # computationcode to work properly.


czwa · 2024-04-22T21:58:02Z

python/lsst/cp/pipe/utils.py

    def estimate_p0(self):
        """Estimate initial fit parameters.

        Returns
        -------
        p0 : `np.ndarray`
            Parameter array, with spline values (one for each node) followed
-            by proportionality constants (one for each group).
+            by proportionality constants (one for each group), and one extra
+            for the offset O (if fitOffset was set to True).


And +2 if fit_weights=True, and +1 if fit_temperature=True, right?

czwa · 2024-04-22T22:04:25Z

python/lsst/cp/pipe/utils.py

        pars : `np.ndarray`
            Parameter array, with spline values (one for each node) followed
-            by proportionality constants (one for each group.)
+            by proportionality constants (one for each group.), followed by
+            (optionally) one offset O.


Similar comment here: I don't think these docstrings were updated after the fit parameters were extended.

czwa · 2024-04-22T22:07:35Z

python/lsst/cp/pipe/utils.py

@@ -1075,7 +1185,7 @@ def fit(self, p0, min_iter=3, max_iter=20, max_rejection_per_iteration=5, n_sigm
        ----------
        p0 : `np.ndarray`
            Initial fit parameters (one for each knot, followed by one for
-            each grouping).
+            each grouping, and optionally one for the offset.)


czwa · 2024-04-22T22:25:32Z

tests/test_linearity.py

@@ -324,6 +373,11 @@ def _check_linearity_spline(self, do_pd_offsets=False, n_points=200):
            self.input_dims,
        ).outputLinearizer

+        if do_weight_fit:
+            # These checks currently fail, and weight fitting is not


Is there a ticket to investigate/fix the weight fitting that could be added here?

czwa · 2024-04-22T22:27:33Z

python/lsst/cp/pipe/linearity.py

+    splineFitWeightParsStart = pexConfig.ListField(
+        dtype=float,
+        doc="Starting parameters for weight fit, if doSplineFitWeights=True. "
+            "Parameters are such that sigma = sqrt(par[0]**2. + par[1]**2./mu).",


I have questions about this, but after reading the rest of the changes (and the test comment), it seems like these aren't being fit as part of the standard pipeline code, so I think my questions aren't relevant at the moment.

Note that currently fitting the weights fails tests; we will have to check on real data and return to see if it's a limitation of the test data or the implementation or the general idea.

…utation.

czwa

A few comments as I worked my way through the fitting process, with only docstring changes I think.

czwa · 2024-05-10T20:37:42Z

python/lsst/cp/pipe/linearity.py

+            "If doSplineFitWeights=False then these are used as-is; otherwise "
+            "they are used as the initial values for fitting these parameters.",
+        length=2,
+        default=[1.0, 0.0],


Am I correct that these are essentially a fixed-error and a signal-to-noise based term?

czwa · 2024-05-10T20:58:40Z

python/lsst/cp/pipe/linearity.py

@@ -365,12 +420,19 @@ def run(self, inputPtc, dummy, camera, inputDims,
            else:
                mask = inputPtc.expIdMask[ampName].copy()

+            if self.config.linearityType == "Spline" and temperatureValues is not None:
+                mask &= np.isfinite(temperatureValues)


Are these NaNs? Do we understand what's going on in these exposures?

czwa · 2024-05-10T21:01:10Z

python/lsst/cp/pipe/linearity.py

-                # Sum[(S(mu_i) + mu_i)/(k_j * D_i) - 1]**2, where S(mu_i) is
-                # an Akima Spline function of mu_i, the observed flat-pair
+                # Sum[(S(mu_i) + mu_i - O)/(k_j * D_i) - 1]**2, where S(mu_i)
+                # is an Akima Spline function of mu_i, the observed flat-pair
                # mean; D_j is the photo-diode measurement corresponding to


czwa · 2024-05-10T21:05:13Z

python/lsst/cp/pipe/linearity.py

                # mean; D_j is the photo-diode measurement corresponding to
                # that flat-pair; and k_j is a constant of proportionality
                # which is over index j as it is allowed to
                # be different based on different photodiode settings (e.g.
-                # CCOBCURR).
+                # CCOBCURR); and O is a constant offset to allow for light


Is this O a single value for all pairs? I'll find out when I get to the fitting code, but I'm just checking if this needs any index as well.

And I agree now that this is O, with no indexing.

czwa · 2024-05-10T21:14:50Z

python/lsst/cp/pipe/linearity.py

+                sigmad = np.nan
+            else:
+                sigmad = median_abs_deviation(residuals[finite]/inputOrdinate[finite], scale="normal")
+            linearizer.fitResidualsSigmaMad[ampName] = sigmad


I'm not clear on the logic here. Is this checking for NaN (in which case any NaN value will result in sigmad=NaN), or is this checking for infinite values (which I see scipy.stats.median_abs_deviation does handle).

czwa · 2024-05-10T21:39:13Z

python/lsst/cp/pipe/utils.py

+        else:
+            mu_corr = mu
+
+        numerator = mu_corr - spl.interpolate(mu_corr)


Does this line contradict the docstrings that list S(mu_i) + mu_i - O? It's also entirely possible I've confused myself.

I think I've convinced myself that I am not confused in this case.

czwa · 2024-05-13T20:39:41Z

I continue to be happy with approval, including the two most recent commits (on failed amp/length matching),

erykoff requested a review from Alex-Broughton April 18, 2024 15:17

czwa approved these changes Apr 22, 2024

View reviewed changes

erykoff force-pushed the tickets/DM-42766 branch from d7494a3 to 612d667 Compare April 26, 2024 18:21

erykoff added 16 commits April 30, 2024 10:31

Add cut for minimum photodiode current.

3535895

Add option to fit offset with spline linearity fit.

ff70322

Add weight parameters to linearity fit.

42fa2e0

Don't warn if readnoise nan is the same as fit nan.

87c1d0d

Rename config to doSplineFitOffset.

873b83b

Add weighted fit and code to fit the weights.

081ae7f

Note that currently fitting the weights fails tests; we will have to check on real data and return to see if it's a limitation of the test data or the implementation or the general idea.

Add temperature coefficient fit option.

3ee6c30

Update linearity pipelines for new features.

c682272

Remove erroneous config addition.

cf16e51

Change grouping to no longer require sorted groups.

2cbb408

Put in new settings for updated linearity fits.

5dd1d29

Use 15 spline knots for LSSTCam linearity fits.

68bc896

Filter possible warnings when checking noise in PTC.

0ddd88c

Ensure temperature/offset is incorporated before spline residual comp…

769a810

…utation.

Remove linearity residual override when using temperature parameter.

a5e7bf8

Add spline chi-squared calculation.

d1d8500

erykoff force-pushed the tickets/DM-42766 branch from 612d667 to d1d8500 Compare April 30, 2024 17:31

erykoff added 3 commits April 30, 2024 11:16

Add linearizer residual sigma_mad measurements.

3b3cabd

Make sure bad amps have fitResidualsSigmaMad set.

91a10ff

Update variable names and docstrings for clarity/completeness.

afb89a8

czwa approved these changes May 10, 2024

View reviewed changes

erykoff added 2 commits May 13, 2024 13:30

Add tests for consistent parameter lengths from amp to amp.

8e79617

Add code to fix up bad amps and ensure equal lengths.

d3d958b

erykoff merged commit 87dad42 into main May 13, 2024
2 checks passed

erykoff deleted the tickets/DM-42766 branch May 13, 2024 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-42766: Update linearity fitting code to fit temperature dependence and other updates recommended by Pierre Astier #239

DM-42766: Update linearity fitting code to fit temperature dependence and other updates recommended by Pierre Astier #239

erykoff commented Apr 18, 2024

czwa left a comment

czwa Apr 22, 2024

czwa Apr 22, 2024

czwa Apr 22, 2024

czwa Apr 22, 2024

czwa Apr 22, 2024

czwa Apr 22, 2024

czwa Apr 22, 2024

czwa left a comment

czwa May 10, 2024

czwa May 10, 2024

czwa May 10, 2024

czwa May 10, 2024

czwa May 10, 2024

czwa May 10, 2024

czwa May 10, 2024

czwa May 10, 2024

czwa commented May 13, 2024

DM-42766: Update linearity fitting code to fit temperature dependence and other updates recommended by Pierre Astier #239

DM-42766: Update linearity fitting code to fit temperature dependence and other updates recommended by Pierre Astier #239

Conversation

erykoff commented Apr 18, 2024

czwa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czwa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czwa commented May 13, 2024