DM-10299: Apply crosstalk correction in decam #50

mrawls · 2018-03-13T23:23:40Z

No description provided.

PaulPrice · 2018-03-14T00:25:59Z

python/lsst/ip/isr/isrTask.py

@@ -465,6 +466,9 @@ def run(self, ccdExposure, bias=None, linearizer=None, dark=None, flat=None, def

        @return a pipeBase.Struct with field:
         - exposure
+        otherDataRef : `lsst.daf.persistence.butlerSubset.ButlerDataRef`
+            optional dataRef, e.g., needed for DECam crosstalk correction


Isn't it possible to derive the exposure-based dataRef from the CCD-based dataRef?
And I believe the run method is forbidden from receiving a dataRef.

You're correct that run methods generally shouldn't receive a dataRef, as per RFC-352. I've added this special case because I need to extract data from other CCDs within DecamCrosstalkTask. I use the dataRef to get a Butler and retrieve one or more different exposures with different ccdnums as needed. It's not clear to me how I could move this into a runDataRef method as suggested by RFC-352, but I'm open to suggestions.

I also thought I'd preserved old-style docstrings here for consistency (this was before the AHM hack day doc-venture), so I'm impressed you found this briefly-existing numpydoc version :)

I think this will work:

def getVisitSensorRefs(sensorRef): """Return a data reference for all sensors in the visit Parameters ---------- sensorRef : `lsst.daf.persistence.ButlerDataRef` Data reference for a single sensor. Returns ------- visitSensorRefs : `list` of `lsst.daf.persistence.ButlerDataRef` Data references for all sensors in the visit. """ butler = sensorRef.getButler() visitKeys = butler.getKeys("raw", "visit") dataId = sensorRef.dataId.copy() for kk in set(dataId.keys()).difference(set(visitKeys)): dataId.pop(kk) visitRef = butler.dataRef("raw", "visit", dataId) return [rr for rr in visitRef.subItems("ccd")]

The established pattern is that runDataRef loads everything that's required and passes that to the run method. I am concerned that it's naive (because loading the entire focal plane at once for every CCD is not good), but them's the rules. You'll need to get @RobertLuptonTheGood to sign off on your design.

Note that the decision on RFC-352 was "We will permit exceptions to that, by means of RFC — anybody who is (re-)designing or implementing a task, and who doesn't feel that run is appropriate, can file an RFC to explain why and get an exemption." We haven't fixed up our existing cases either.

Your suggested getVisitSensorRefs() seems to have its own problems of butler dataId manipulation.

What's the conclusion here? I agree with John that there are cases when we might need to pass objects that look a bit like dataRefs around (reference catalogues are one), but I didn't see an RFC to permit it here.

Why is this in DecamCrosstalkTask rather than generalising the cross-talk correction code?

Hey folks — just to be clear, the aim behind this PR was for @mrawls to iterate with @SimonKrughoff on the approach she should take here. As of now, that discussion is still ongoing; let's see how it converges. Should any RFCs or other special measures be needed, rest assured that everything will be done by the book.

I believe the conclusion is to add a (noop) method called prepCrosstalk to the default CrosstalkTask. Then they will override that in the DECam version to assemble need information. That's a generic solution that other crosstalk implementations can use.

SimonKrughoff

There may be another argument to the run method, but I don't think it has to be a data ref.

SimonKrughoff · 2018-03-17T00:04:56Z

python/lsst/ip/isr/isrTask.py

@@ -439,7 +439,8 @@ def readIsrData(self, dataRef, rawExposure):
    def run(self, ccdExposure, bias=None, linearizer=None, dark=None, flat=None, defects=None,
            fringes=None, bfKernel=None, camera=None,
            opticsTransmission=None, filterTransmission=None,
-            sensorTransmission=None, atmosphereTransmission=None):
+            sensorTransmission=None, atmosphereTransmission=None,
+            otherDataRef=None):


The way you've implemented the decam crosstalk correction, I don't think that modifications here are necessary. I think this should be reverted except to add a prepCrosstalk method to the CrosstalkTask base class.

That method would be called in the RunDataRef method on IsrTask. You would still have to pass through the source data object, but then it wouldn't be opaque.

SimonKrughoff · 2018-03-23T17:50:10Z

python/lsst/ip/isr/isrTask.py

        # immediate=True required for functors and linearizers are functors; see ticket DM-6515
        linearizer = dataRef.get("linearizer", immediate=True) if self.doLinearize(ccd) else None
        darkExposure = self.getIsrExposure(dataRef, "dark") if self.config.doDark else None
-        flatExposure = self.getIsrExposure(dataRef, "flat") if self.config.doFlat else None
+        flatExposure = self.getIsrExposure(dataRef, self.config.flatDataProductName) \
+            if self.config.doFlat else None


I'm confused why this is in a ticket for adding crosstalk. It seems to be complicating the issue.

Also, it seems odd to only generalize two of them. Why not also generalize dark?

This came up when I was writing a test which calls DecamCpIsrTask, which had its own copy-pasted readIsrData method for the specially-named cpBias and cpFlat issue alone. It was slowly diverging from ip_isr's readIsrData which is what I wanted to use anyway, so instead of perpetuating the copy-paste cycle I thought it best to just add this fix. (There aren't such a thing as cpDarks so I let it be.)

I see. Please add the generalization for dark. I think it would be pretty confusing to someone who uses this to have access to only two of the three.

SimonKrughoff · 2018-03-23T17:53:24Z

python/lsst/ip/isr/isrTask.py

-                self.crosstalk.run(ccdExposure, otherDataRef)
+            if crosstalkSources is not None:
+                # crosstalkSources is used by DECam
+                self.crosstalk.run(ccdExposure, crosstalkSources)
            else:
                self.crosstalk.run(ccdExposure)


I think you should add the crosstalkSources to the default CrosstalkTask. It can default to None in the base class.

Think I've got this now.

SimonKrughoff

I'm happy with this if the one simplification I mention is implemented.

SimonKrughoff · 2018-03-26T19:11:29Z

python/lsst/ip/isr/isrTask.py

+                # crosstalkSources is used by DECam
+                self.crosstalk.run(ccdExposure, crosstalkSources)
+            else:
+                self.crosstalk.run(ccdExposure)


This can now be collapsed to:

if self.config.doCrosstalk: self.crosstalk.run(ccdExposure, crosstalkSources)

Note that only obs_decam presently has a prepCrosstalk method which returns crosstalkSources that are not None. Correspondingly, CrosstalkTask's run now accepts crosstalkSources but expects them to be None by default. Includes some flake8 cleanups and numpydoc docstrings.

This is useful for using community pipeline bias and flats with obs_decam, which are called cpBias and cpFlat. For completeness, we include darkDataProductName too.

PaulPrice reviewed Mar 14, 2018

View reviewed changes

SimonKrughoff suggested changes Mar 17, 2018

View reviewed changes

SimonKrughoff reviewed Mar 23, 2018

View reviewed changes

mrawls force-pushed the tickets/DM-10299 branch from ba25570 to daf9f15 Compare March 23, 2018 21:57

Move crosstalk correction to before CCD assembly

cc1e91f

mrawls force-pushed the tickets/DM-10299 branch from daf9f15 to c17ebf1 Compare March 23, 2018 23:15

SimonKrughoff approved these changes Mar 26, 2018

View reviewed changes

mrawls force-pushed the tickets/DM-10299 branch from c17ebf1 to 5990cfb Compare March 26, 2018 19:21

mrawls added 2 commits March 26, 2018 12:24

Add calib- DataProductName configs

e427493

This is useful for using community pipeline bias and flats with obs_decam, which are called cpBias and cpFlat. For completeness, we include darkDataProductName too.

mrawls force-pushed the tickets/DM-10299 branch from 5990cfb to e427493 Compare March 26, 2018 19:25

mrawls merged commit e427493 into master Mar 26, 2018

ktlim deleted the tickets/DM-10299 branch August 25, 2018 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-10299: Apply crosstalk correction in decam #50

DM-10299: Apply crosstalk correction in decam #50

mrawls commented Mar 13, 2018

PaulPrice Mar 14, 2018

mrawls Mar 14, 2018

PaulPrice Mar 14, 2018 •

edited

PaulPrice Mar 14, 2018

parejkoj Mar 15, 2018

RobertLuptonTheGood Mar 19, 2018

jdswinbank Mar 19, 2018

SimonKrughoff Mar 19, 2018

SimonKrughoff left a comment

SimonKrughoff Mar 17, 2018

SimonKrughoff Mar 17, 2018

SimonKrughoff Mar 23, 2018

mrawls Mar 23, 2018

SimonKrughoff Mar 23, 2018

SimonKrughoff Mar 23, 2018

mrawls Mar 23, 2018

SimonKrughoff left a comment

SimonKrughoff Mar 26, 2018

DM-10299: Apply crosstalk correction in decam #50

DM-10299: Apply crosstalk correction in decam #50

Conversation

mrawls commented Mar 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulPrice Mar 14, 2018 • edited

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

SimonKrughoff 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

SimonKrughoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulPrice Mar 14, 2018 •

edited