DM-23044: PTC task should persist usable linearity models #29

plazas · 2020-03-28T17:05:26Z

No description provided.

timj

I have quite a few questions and comments. Some of my questions are my lack of understanding of what Linearizer does.

timj · 2020-03-30T18:39:39Z

python/lsst/cp/pipe/ptc.py

+        now = datetime.datetime.utcnow()
+        butler = dataRef.getButler()
+
+        for linPair in [("LOOKUPTABLE", 'linearizeLut'), ("LINEARIZEPOLYNOMIAL", 'linearizePolynomial'),


change this to

for linType, dataType in [("LOOKUPTABLE", 'linearizeLut'), ("LINEARIZEPOLYNOMIAL", 'linearizePolynomial')]:

timj · 2020-03-30T18:41:54Z

python/lsst/cp/pipe/ptc.py

+                                                    instruName=self.config.instrumentName,
+                                                    linearizerType=linType, tableArray=tableArray,
+                                                    log=self.log)
+            butler.put(linearizer.toDict(), datasetType=dataType, dataId={'detector': detNum,


Why are we putting a dict here and not the actual linearizer object? I don't really understand gen2 so maybe I'm getting confused but shouldn't it be able to take a special object?

I tried getting rid of "toDict" and it did not work properly. It seems that gen2 butler expects a dict.

You would need to teach it to do the right thing like we taught butler to understand that Defects can be put.

I see that cp_pipe doesn't use the butler for defects but does try to use the butler for ptc. I think that explains why Butler.put has not been set up for Defects. It should be simple for Linearizer to have the dataset type added to gen2 butler since it emulates a normal Catalog by supporting writeFits.

So the linearizer should be treated like a fits catalog and written as a fits catalog? Should not they be human-readable?

You are putting them into a butler so the format does not have to be the same as the format you use in obs_lsst_data. If you have been copying the files out of the butler manually then that would make a difference (otherwise you'd need a quick readFits/writeText copy-like command). I don't know how easy it is to add a new formatter in gen2 that would call writeText directly so maybe it's not going to be possible to do this in gen2 properly.

At this moment I don't know either how easy what you describe would be. Could this be left for a future ticket, if necessary?

timj · 2020-03-30T18:43:50Z

python/lsst/cp/pipe/ptc.py

+            Detector object
+        instruName : `str`, optional
+            Instrument name
+        linearizerType : `str`, optional


Given that you default linearizerType argument to an empty string and then raise an exception if you give an empty string, doesn't that mean that the parameter is not optional at all?

Yes. I set "LINEARIZEPOLYNOMIAL" as default.

Is that a safe default to use? Are you better off always being explicit?

timj · 2020-03-30T18:44:27Z

python/lsst/cp/pipe/ptc.py

+            Detector name
+        detNum : `int`
+            Detector number
+        detector : `lsst.afw.cameraGeom.detector.detector.Detector`


I think this should be lsst.afw.cameraGeom.Detector

Also, don't pass in detNum and detName if you are already passing in the entire detector object. Calculate what you need directly in this method.

timj · 2020-03-30T18:44:35Z

python/lsst/cp/pipe/ptc.py

+        ----------
+        dataset : `lsst.cp.pipe.ptc.PhotonTransferCurveDataset`
+            The dataset containing the means, variances and exposure times
+        detName : `srt`


str not srt.

timj · 2020-03-30T19:43:25Z

python/lsst/cp/pipe/ptc.py

+                linearizer = Linearizer(table=tableArray, log=log)
+            else:
+                raise RuntimeError("tableArray must be provided when creating a LookupTable linearizer")
+        elif linearizerType in ['LINEARIZESQUARED', 'LINEARIZEPOLYNOMIAL']:


In general using a list for a readonly sequence seems odd to me and a tuple might be better. Also be consistent with quote type. You mostly seem to use double quotes so use them everywhere.

timj · 2020-03-30T19:50:55Z

python/lsst/cp/pipe/ptc.py

+            ampName = amp.getName()
+            if linearizerType == 'LOOKUPTABLE':
+                linearizer.linearityCoeffs[ampName] = [i, 0]
+                linearizer.linearityType[ampName] = "LookupTable"


A way to remove the linearity type from the if clauses would be to define the mapping in a dict:

linearityType = {"LOOKUPTABLE": "LookupTable", "LINEARIZESQUARED": "Squared"} for ... in ...: linearizer.linearityType[ampName] = linearityType[linearizerType]

Saying that, why do you need to tell the Linearizer object what each amp is when it seems to be a constant? Am I seeing a special case here or is it always true that every amp has the same type? Could you instead have Linearizer subclasses that have their type defined for them like @SimonKrughoff did for the Curve class? Maybe @czwa can tell me. Of course if each amp can have different types then that would explain this but the code here doesn't allow that.

Having this be amplifier based instead of detector (or camera) based is a result of directly mapping the cameraGeom linearity parameters to the new Linearizer. I also didn't want to prevent different amplifiers from having different correction types (because electronics can always be difficult in unique ways).

So this means that Linearizer can support per-amp but this cp_pipe task can't. Can this be mentioned somewhere in the task docstring?

I left the "if" clauses because there are other things specific to each linearizer type that are being set. I added clarification regarding the linearizers in the doc-string of "MeasurePhotonTransferCurveTask".

timj · 2020-03-30T19:54:17Z

python/lsst/cp/pipe/ptc.py

            meanSignalVector = k0 + k1*exposureTimeVector + k2*exposureTimeVector^2 +...
                               + kn*exposureTimeVector^n, with n = "polynomialFitDegreeNonLinearity".

-        linearizerTableRow: list of `np.float`
+        linearizerTableRow : list of `np.float`


Use back ticks. I think np.float and float are interchangeable so do they really need to be numpy floats?

timj · 2020-03-30T19:55:19Z

python/lsst/cp/pipe/ptc.py

@@ -623,55 +735,76 @@ def calculateLinearityResidualAndLinearizers(self, exposureTimeVector, meanSigna

        Returns
        -------
-        c0: `np.float`
+        polynomialLinearizerCoefficients : `list` of `np.float`


Do not use np abbreviation in docstrings. numpy.float should be used. Why is this a list of individual numpy floats and not a single numpy array? (and also float and numpy.float should be identical from your point of view shouldn't they?)

timj · 2020-03-30T19:57:00Z

python/lsst/cp/pipe/ptc.py

        if ptcFitType == 'ASTIERAPPROXIMATION':
            ptcFunc = self.funcAstier
-            stringTitle = r"Var = $\frac{1}{2g^2a_{00}}(\exp (2a_{00} \mu g) - 1) + \frac{n_{00}}{g^2}$"
-
+            stringTitle = (r"Var = $\frac{1}{2g^2a_{00}}(\exp (2a_{00} \mu g) - 1) + \frac{n_{00}}{g^2}$ " +


Do not use a plus to concatenate. It's not needed.

timj · 2020-03-30T20:03:23Z

python/lsst/cp/pipe/ptc.py

+        """
+        if linearizerType == 'LOOKUPTABLE':
+            if tableArray is not None:
+                linearizer = Linearizer(table=tableArray, log=log)


Would it help if the detector argument was passed in here?

plazas requested a review from timj March 28, 2020 17:13

timj requested changes Mar 30, 2020

View reviewed changes

timj reviewed Mar 30, 2020

View reviewed changes

plazas added 26 commits April 2, 2020 11:56

Test writeText for LUT linearizer

851534e

Test persisting linearizeLut table

bf40796

Persist lut and linearizerLut

fc6855a

Populate metadata of output linearizer

a2b27bc

Save LUT linearizer yaml files

dccd102

Persist squared linearizer

688911b

Add detector serial and fix bugs

b5242ae

Add polynomial linearizer calculation

d326966

Clarify that red. chi sq is unweighted

5cf83b2

Rearrange arguments in fitPtcAndNonLinearity

ce98855

Change variables in for loop

a52d60d

Set default for linearizerType

d4a0e59

Delete detNum and detName: redundant

0a9a990

Turn array into tuple

dfcd79c

Delete 'filter'

4797c2d

Delete 'raftName'

a34cfff

Define calibDate once and pass it to method

695016b

Make linearizerType not optional

466c647

Lengthen log message

2afe40f

Adjust error message

4611549

Add elif

fd0fa9a

Add clarification about linearizers to docstring

84828ac

Switch from np.float to float in doctrings

504faf1

Add detector as argument to Linearizer

3040a8c

Add filter and raftName back to calibId

731136c

Switch ADU to DN

0860c17

plazas force-pushed the tickets/DM-23044 branch from 7db3e20 to 0860c17 Compare April 2, 2020 17:00

plazas added 2 commits April 3, 2020 08:59

Make squared coefficient a list

c9210d6

Check correction coefficients

f12bc7a

plazas merged commit 57ef05f into master Apr 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-23044: PTC task should persist usable linearity models #29

DM-23044: PTC task should persist usable linearity models #29

plazas commented Mar 28, 2020

timj left a comment

timj Mar 30, 2020

timj Mar 30, 2020

plazas Mar 31, 2020

timj Mar 31, 2020

timj Mar 31, 2020

plazas Mar 31, 2020

timj Mar 31, 2020

plazas Apr 1, 2020

timj Mar 30, 2020

plazas Mar 31, 2020

timj Mar 31, 2020

timj Mar 30, 2020

timj Mar 30, 2020

timj Mar 30, 2020

timj Mar 30, 2020

timj Mar 30, 2020

czwa Mar 31, 2020

timj Mar 31, 2020

plazas Apr 1, 2020

timj Mar 30, 2020

timj Mar 30, 2020

timj Mar 30, 2020

timj Mar 30, 2020

DM-23044: PTC task should persist usable linearity models #29

DM-23044: PTC task should persist usable linearity models #29

Conversation

plazas commented Mar 28, 2020

timj 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

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

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