DM-24389: Implement amp-to-amp offset correction task #173
Conversation
leeskelvin
force-pushed
the
tickets/DM-24389
branch
from
5b62f56
to
4e281e6
Compare
python/lsst/ip/isr/ampOffset.py
Outdated
| ) | ||
| ampEdgeMaxStep = pexConfig.Field( | ||
| doc="Maximum allowed amp-to-amp step value. If a measured amp-to-amp step value is larger than this, " | ||
| "the result will be discarded and therefore not used to determine amp offset piston corrections.", |
There was a problem hiding this comment.
We have (starting here in my review) the use of amp-to-amp step and amp offset piston. I think we need to (a) standardize on a name, and (b) the more I think about it I don't like piston because it confused Josh (there's a physical up-down piston which relates to focus). I'm not sure what name to use, maybe we should open this up on #dm-naming-things.
There was a problem hiding this comment.
The doc string should specify the units (ADU).
There was a problem hiding this comment.
Thanks for these comments Eli, and apologies it's taken me some time to reply. So, following your comments here and the previous discussion on #dm-naming-things, I have now standardized on two names to use for this ticket: amp offset describes the jump (in ADU) from one amp to another, and amp pedestal describes the specific value (in ADU) added/subtracted from an individual amplifier. All mentions of 'step' and 'piston' have been removed from the code and associated doc strings. I hope that reads well, but keen to hear your thoughts if not.
There was a problem hiding this comment.
PS - mention of units (ADU) added to the docstring here also, thank you.
python/lsst/ip/isr/ampOffset.py
Outdated
|
|
||
| def run(self, exposure): | ||
| """Calculate amp-to-amp step values, determine corrective piston | ||
| pedestals for each amp, and update the input exposure in-place. This |
There was a problem hiding this comment.
And now we have piston pedestals. We want to make sure this is consistent everywhere.
There was a problem hiding this comment.
Following above, this opening sentence now reads: Calculate amp offset values, determine corrective pedestals for each amp, and update the input exposure in-place.
| @@ -721,6 +721,17 @@ class IsrTaskConfig(pipeBase.PipelineTaskConfig, | |||
| default=True, | |||
| ) | |||
|
|
|||
| # Amp offset correction. | |||
| doAmpOffset = pexConfig.Field( | |||
There was a problem hiding this comment.
This and below might be renamed, but I'm actually satisfied with this. But we should ask more broadly. I just want to make sure that a search of the codebase will come up with something consistent and relatively unique. SEO style!
There was a problem hiding this comment.
This name has been preserved.
leeskelvin
force-pushed
the
tickets/DM-24389
branch
2 times, most recently
from
c0084ae
to
a1e74fe
Compare
leeskelvin
force-pushed
the
tickets/DM-24389
branch
from
a1e74fe
to
e66428f
Compare
leeskelvin
force-pushed
the
tickets/DM-24389
branch
from
e66428f
to
930dd8e
Compare
leeskelvin
force-pushed
the
tickets/DM-24389
branch
from
930dd8e
to
92ba8bf
Compare
leeskelvin
force-pushed
the
tickets/DM-24389
branch
from
92ba8bf
to
d782671
Compare
This commit adds a placeholder amp offset task which is not implemented here, and instead should be retargeted by a camera specific version.
leeskelvin
force-pushed
the
tickets/DM-24389
branch
from
d782671
to
481dcfb
Compare
This commit adds a placeholder amp offset task which is not implemented
here, and instead should be retargeted by a camera specific version.