DM-29326: Fix logic of the doVignette option in isrTask #203
Conversation
python/lsst/ip/isr/vignette.py
Outdated
| """ | ||
| ConfigClass = VignetteConfig | ||
| _DefaultName = "isrVignette" | ||
|
|
||
| def run(self, exposure): | ||
| def run(self, exposure=None, doUpdateMask=True, maskPlane="NO_DATA", vignetteValue=None, log=None): | ||
| """Generate circular vignette pattern. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| exposure : `lsst.afw.image.Exposure` |
There was a problem hiding this comment.
This exposure parameter seems now optional, so it should be indicated as such in the doc string. In addition, the new parameters should also be documented in the docstring.
python/lsst/ip/isr/vignette.py
Outdated
| def setValidPolygonCcdIntersect(ccdExposure, fpPolygon, log=None): | ||
| """Set valid polygon on ccdExposure associated with focal plane polygon. | ||
|
|
||
| Where the ccd exposure's valid polygon is considered the intersection of |
There was a problem hiding this comment.
Maybe rephrase a bit with something like: "The CCD exposure's valid polygon is defined as the intersection of ... "
python/lsst/ip/isr/isrTask.py
Outdated
| @@ -859,9 +857,21 @@ class IsrTaskConfig(pipeBase.PipelineTaskConfig, | |||
| # Vignette correction configuration. | |||
| doVignette = pexConfig.Field( | |||
| dtype=bool, | |||
| doc="Apply vignetting parameters?", | |||
| doc="Compute and attach validPolygon to exposure according to vignetting parameters?", | |||
There was a problem hiding this comment.
It's not that clear here if a "validPolygon" is some sort of formal type, or if it is defined as something else. I know we should try to be concise here, but if possible, perhaps you could add a few more phrases to explain what "validPolygon" is for someone who looks at this doc.
python/lsst/ip/isr/isrTask.py
Outdated
| ) | ||
| vignetteValue = pexConfig.Field( | ||
| dtype=float, | ||
| doc="Value to replance image array pixels with in the vingetted region? Ignored if None.", |
There was a problem hiding this comment.
Totally inherited from me...sorry! And I'm also questioning my decision to add a question mark here!
|
Hey Chris, just in case you haven't noted this already, you may want to remove the WIP: from the summary of any commit messages that are indeed getting merged (i.e. no longer WIP!) |
czwa
force-pushed
the
tickets/DM-29326
branch
from
ce7913e
to
029edd3
Compare
There's no point in continuing if the filter list is empty.
- Fix docstrings. - Make a missing polygon do no masking instead of full masking. - Update logging method.
This is now ignored and deprecated, so it will warn. This should help track down any existing cases where it's used.
czwa
force-pushed
the
tickets/DM-29326
branch
from
029edd3
to
7774b6e
Compare
Logic changed, and masking function updated so it can be used generically.