Add support for the new brighter-fatter kernels #56
Conversation
mfisherlevine
force-pushed
the
tickets/DM-13293
branch
from
027979f
to
4892439
Compare
python/lsst/ip/isr/isrTask.py
Outdated
| @@ -417,7 +425,7 @@ def readIsrData(self, dataRef, rawExposure): | |||
| if self.config.doDark else None | |||
| flatExposure = self.getIsrExposure(dataRef, self.config.flatDataProductName) \ | |||
| if self.config.doFlat else None | |||
| brighterFatterKernel = dataRef.get("brighterFatterKernel") if self.config.doBrighterFatter else None | |||
| brighterFatterKernel = dataRef.get("bfKernelNew") if self.config.doBrighterFatter else None | |||
There was a problem hiding this comment.
I thought you decided to use "brighterFatterKernel"
There was a problem hiding this comment.
Indeed, this was just stale, thanks.
python/lsst/ip/isr/isrTask.py
Outdated
| @@ -574,6 +582,10 @@ def run(self, ccdExposure, bias=None, linearizer=None, dark=None, flat=None, def | |||
| self.maskAndInterpNan(ccdExposure) | |||
| interpolationDone = True | |||
|
|
|||
| if self.config.brighterFatterLevel == 'CCD': | |||
There was a problem hiding this comment.
Fixed here, and in the config option default and doc.
| if self.config.brighterFatterLevel == 'CCD': | ||
| bfKernel = bfKernel[ccdExposure.getDetector().getId()] | ||
| else: | ||
| raise NotImplementedError("per-amplifier brighter-fatter correction not yet implemented") |
There was a problem hiding this comment.
Is this outdated? Your new code in cp_pipe does support per-amplifier brighter-fatter correction.
There was a problem hiding this comment.
It's actually not outdated, no. The code there generates kernels at the per-amplifier level, however this is the correction code, and nothing has yet been written to support doing the correction at the per-amplifier level. If it looked trivial I would have chucked it in with this ticket, but because the detector is already assembled at this point in the code it's not quite that easy. I will file a ticket saying that this needs to be added at some point though.
mfisherlevine
force-pushed
the
tickets/DM-13293
branch
from
f303319
to
c561fd9
Compare
|
I think I've done everything I should have, so please consider this the updated version to take a look at. |
mfisherlevine
force-pushed
the
tickets/DM-13293
branch
from
80d1a86
to
c6d4c88
Compare
python/lsst/ip/isr/isrTask.py
Outdated
| @@ -585,6 +594,11 @@ def run(self, ccdExposure, bias=None, linearizer=None, dark=None, flat=None, def | |||
| self.maskAndInterpNan(ccdExposure) | |||
| interpolationDone = True | |||
|
|
|||
| if self.config.brighterFatterLevel == 'DETECTOR': | |||
| bfKernel = bfKernel[ccdExposure.getDetector().getId()] | |||
There was a problem hiding this comment.
You are replacing bfKernel with an element from the variable of the same name. I think this would be a lot less confusing if you used a different variable name for the sequence than for the element of the sequence, e.g. kernelElement = brighterFatterKernel[...]
It would also be less confusing if the dataset type name was not singular (e.g. brighterFatterKernels) since you are getting a kernel from it. But I suspect it's too late to reasonably do anything about that.
There was a problem hiding this comment.
Also, as a minor point: since "bf" is expanded almost everywhere else, the code would be a bit easier to read if you used "brighterFatter" for the kernel variable as well -- especially as it matches a dataset type.
Of course it gets long if you need two names for the kernel vs. the collection of kernels...perhaps just leave out "bf" for the former, e.g. detectorKernel = brighterFatterKernel[...]
There was a problem hiding this comment.
Oh good lord, that's a great spot there, overwriting the object with an element of it, that was totally unintended! I have changed that now.
Regarding the rest, I take your point, but sadly the name bfKernel is passed in as a kwarg so changing that is RFC territory (in reality I'm sure it's fine, but technically I think it's required?). So as you say, I don't think it's worth changing now, especially as all of isr will need a cleanup at some point.
| bfKernel = bfKernel[ccdExposure.getDetector().getId()] | ||
| else: | ||
| # TODO: DM-15631 for implementing this | ||
| raise NotImplementedError("per-amplifier brighter-fatter correction not yet implemented") |
There was a problem hiding this comment.
Do you anticipate being able to call self.brighterFatterCorrection in a single pass for the per-amplifier case? If not, I suggest putting the following into the "if" above.
mfisherlevine
force-pushed
the
tickets/DM-13293
branch
from
c6d4c88
to
fee7246
Compare
mfisherlevine
force-pushed
the
tickets/DM-13293
branch
from
d5f86a2
to
51e7290
Compare
mfisherlevine
force-pushed
the
tickets/DM-13293
branch
from
51e7290
to
b14fc9a
Compare
bfKernelNew is a temporary name for this dataset
while the crossover between old and new is handled
This is largely irrelevant though, as this code has
never fired before, as only obs_subaru actually
brighter-fatter correction, and it has its own isr
task. This will be updated once this dataset is
renamed.