DM-22776: Update Gen3 isrTask for BF corrections #124
Conversation
| # the information as a numpy array. | ||
| if self.config.doBrighterFatter: | ||
| brighterFatterKernel = inputs.pop('newBFKernel', None) | ||
| if brighterFatterKernel is None: |
There was a problem hiding this comment.
why do this line, the if statement below will never be triggered by the bfKernel as it is an array
There was a problem hiding this comment.
If a newBFKernel exists in the inputs, then we should use that (and do the block beginning on line 848 to pull the kernel for the detector we have). Otherwise, we should use the old style, which does not need to do the L848 block (and inputs['bfKernel'] remains what we received from the butler).
|
|
||
| if brighterFatterKernel is not None and not isinstance(brighterFatterKernel, numpy.ndarray): | ||
| detId = detector.getId() | ||
| inputs['bfGains'] = brighterFatterKernel.gain |
There was a problem hiding this comment.
bfGains does not seem to be populated if not newBFKernel, is that what you want?
There was a problem hiding this comment.
That is correct. bfGains will not be in inputs, will then be None inside run(), and will be passed as such to the correction code (which handles the None case by using camera gain values).
python/lsst/ip/isr/isrTask.py
Outdated
| @@ -943,7 +969,7 @@ def readIsrData(self, dataRef, rawExposure): | |||
| # If using a new-style kernel, always use the self-consistent | |||
| # gains, i.e. the ones inside the kernel object itself | |||
| brighterFatterKernel = dataRef.get("brighterFatterKernel") | |||
| brighterFatterGains = brighterFatterKernel.gain | |||
| brighterFatterGains = getattr(brighterFatterKernel, 'gain', None) | |||
There was a problem hiding this comment.
not that its bad, but why getattr here? if it is a new style then gain will be an attribute, if its an old style an exception will be thrown (likely by the dataRef.get) and this block wont be executed. I would be more worried about getting this far if it is an old style. If you wanted to be safe, then maybe a different except block? This seems like it will just hide a problem
There was a problem hiding this comment.
There is a possibility that there will be no .gains attribute, depending on how the kernel was generated. I will ask @mfisherlevine if this is still possible.
There was a problem hiding this comment.
In short, yes, Chris is right, that is technically possible. However, I think that's a bug. The generating code always has them, and downstream code should be able to rely upon them being stored in the kernel so the gains can be applied in a consistent manner, so I think the best course of action here is to allow yourselves to rely up this, and ticket fixing up the BF code so it always stores the gain. The patch should be trivial. Does that sound OK?
There was a problem hiding this comment.
DM-23273 requests that gain existence be enforced. Marking this done, and will merge after testing.
czwa
force-pushed
the
tickets/DM-22776
branch
from
fb28bf9
to
eb244cf
Compare
Update isrTask runQuantum for new-style BF correction. This requires examining the object returned from the butler and reworking it to match the expectation of IsrTask().run().