DM-27010: Allow IsrTask to use PTC dataset as gain source #181

plazas · 2021-04-21T21:16:40Z

No description provided.

mfisherlevine · 2021-04-21T22:23:12Z

python/lsst/ip/isr/isrFunctions.py

@@ -753,14 +753,19 @@ def applyGains(exposure, normalizeGains=False):
    normalizeGains : `Bool`, optional
        If True, then amplifiers are scaled to force the median of
        each amplifier to equal the median of those medians.
+    ptcGains : `dict`[`str`], optional
+        Dictionary keyed by amp number containing the PTC gains.


From the looks of the rest of the code, I think this is actually the amp name, not the amp number (and that's better, much less ambiguity/potential for iteration mistakes that way), so I think just change the doc here.

mfisherlevine · 2021-04-21T22:26:48Z

python/lsst/ip/isr/isrTask.py

@@ -1198,7 +1216,7 @@ def readIsrData(self, dataRef, rawExposure):
    @pipeBase.timeMethod
    def run(self, ccdExposure, camera=None, bias=None, linearizer=None,
            crosstalk=None, crosstalkSources=None,
-            dark=None, flat=None, bfKernel=None, bfGains=None, defects=None,
+            dark=None, flat=None, ptc=None, bfKernel=None, bfGains=None, defects=None,


This is a tricky one. Personally, I think the first line of args should read:
def run(self, ccdExposure, *, camera=None... so that we're mandating everything else to be a kwarg, because anyone trying to use this method trusting on position is asking for trouble. However, putting that in now might well break stuff in other places. So, you could move this arg to the end of the list, or you could put in the * and be careful to run a full Jenkins including the CI packages etc to make sure nothing actually breaks. I'd prefer doing it that way, but understand if you want to just move this to the end of the list. But it's got to be one of the two in order not to potentially break people doing everything positionally.

Quite curious what @czwa thinks is the right approach here. For functions with horrendous numbers of args we should be mandating * really, right?

FWIW, the * option at least passes the ip_isr tests...

(but isrTask.py has the 2nd worst coverage in the whole package)

Putting * sounds reasonable to me, but I'll also wait for Chris to comment. What other packages should be included in Jenkins? I usually use the defaults (lsst_distrib lsst_ci) + ci_cpp_gen3. I guess I can also do ci_cpp_gen2. Anything else?

I think ci_hsc too, basically just thinking we should exercise anything/everything we have that actually runs isr to check (as best we can) that we're not breaking things in the wild. To be honest though, anyone who does have this break stuff for them was playing a dangerous game already I think, so I don't think it's too big of a deal, but certainly worth running anything we can that uses isr.

I usually add ci_hsc and ci_cpp_gen3 to the defaults.

As for the actual question, I think adding the * is probably worth the three extra characters. I would hope no one is relying on position for this method, and the standard case has the arguments passed automatically from runQuantum.

mfisherlevine · 2021-04-21T22:38:03Z

python/lsst/ip/isr/isrTask.py

+        if self.config.usePtcGains and ptcDataset is not None:
+            gain = ptcDataset.gain[amp.getName()]
+        else:
+            gain = amp.getGain()


One of two things needs to happen here: either way, that first if statement needs to be split to check if we're using PTC gains, and then do the if None part. What we need to decide here is, if usePtcGains is True and ptcDataset is None, do we log a warning and use the amp gains, or do we raise? I think I quite strongly prefer the raising option, because if you're specifically saying to use them, and can't, that should be a raise (for comparison, you can't say doBias and have that not present, so I think this should behave the same), but if there's an argument for logging a warning and defaulting to amp gains, I'm open to hearing it for sure.

I was following the example that was there in which if "if self.config.doEmpiricalReadNoise and overscanImage is not None: " failed then it did not raise, but just defaulted to the amp read noise. So I thought the behavior should be similar.

So I'm thinking of doing the first "if" split that you suggest (checking if the user wants to use PTC gains), and if not, then defaulting to amp gains, with a warning.

Honestly, I'm leaning quite strongly the other way, and think that the doEmpiricalReadNoise thing is doing it incorrectly as well. At the end of the day, a warning is a single line in a (sadly) pretty cluttered output, which is easily missed, and if someone is asking for something specific like this, and it can't happen, then I think that should raise. There are very few examples in our pipelines when we ignore specified options and just continue, and I think that this is a particularly bad place to be doing so, because it'll likely usually make a small enough difference that it might not even be noticed downstream, but if people are using it, they'll be doing so for a reason. @RobertLuptonTheGood and @czwa, what are your thoughts here?

Assuming we do move to a raise here, I think we should ticket changing the way doEmpiricalReadNoise works too (or just do it on this ticket?)

mfisherlevine · 2021-04-21T22:39:40Z

python/lsst/ip/isr/isrTask.py

+        elif self.config.usePtcReadNoise and ptcDataset is not None:
+            readNoise = ptcDataset.noise[amp.getName()]
+            self.log.info("Using read noise from Photon Transfer Curve.")


Same logic as above - if people are asking for that, I think it should be a raise if they're not present, rather than just quietly proceeding.

mfisherlevine · 2021-04-22T16:46:48Z

python/lsst/ip/isr/isrTask.py

+        Raises
+        ------
+        RuntimeError
+            Raised is either ``usePtcGains`` of ``usePtcReadNoise``


if, not is.

mfisherlevine · 2021-04-22T16:47:26Z

python/lsst/ip/isr/isrTask.py

+        ------
+        RuntimeError
+            Raised is either ``usePtcGains`` of ``usePtcReadNoise``
+            are ``True``, but there is not ptcDataset provided.


no, not "not" (or "but ptcDataset is not provided", that's also fine.)

plazas added 4 commits April 20, 2021 16:18

Add option to use gains and noise from PTC

0b6a0fe

Fix name or argument in run

a099d57

Fix typo in applyGains

d6c389f

Add log message when using PTC read noise

79f90c7

mfisherlevine reviewed Apr 21, 2021

View reviewed changes

Fix docstring

eb12741

mfisherlevine reviewed Apr 22, 2021

View reviewed changes

plazas added 3 commits April 22, 2021 13:59

Raise if ptcDatset or overscan image are None

76c0a72

Fix typos in docstring

439eed0

Mandate non-positional args in run to be kwargs

2b0fd27

plazas force-pushed the tickets/DM-27010 branch from 2e411b9 to 2b0fd27 Compare April 22, 2021 19:00

plazas merged commit fb99f82 into master Apr 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-27010: Allow IsrTask to use PTC dataset as gain source #181

DM-27010: Allow IsrTask to use PTC dataset as gain source #181

plazas commented Apr 21, 2021

mfisherlevine Apr 21, 2021

mfisherlevine Apr 21, 2021

mfisherlevine Apr 21, 2021

mfisherlevine Apr 21, 2021

mfisherlevine Apr 21, 2021

plazas Apr 22, 2021

mfisherlevine Apr 22, 2021

czwa Apr 22, 2021

mfisherlevine Apr 21, 2021

plazas Apr 22, 2021 •

edited

plazas Apr 22, 2021

mfisherlevine Apr 22, 2021

mfisherlevine Apr 21, 2021

mfisherlevine Apr 22, 2021

mfisherlevine Apr 22, 2021

DM-27010: Allow IsrTask to use PTC dataset as gain source #181

DM-27010: Allow IsrTask to use PTC dataset as gain source #181

Conversation

plazas commented Apr 21, 2021

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

plazas Apr 22, 2021 • 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

plazas Apr 22, 2021 •

edited