DM-28600: Add option to apply skyCorr to ProcessBrightStarsTask #465
Conversation
| code extends over the entire focal plane, this can produce | ||
| better sky subtraction. | ||
| The calexp is updated in-place. | ||
| Parameters |
There was a problem hiding this comment.
Fix spacing (i.e. lack thereof) in docstrings.
There was a problem hiding this comment.
Still need a space after title description.
| """ | ||
| if isinstance(calexp, afwImage.Exposure): | ||
| calexp = calexp.getMaskedImage() | ||
| calexp -= skyCorr.getImage() |
There was a problem hiding this comment.
Worth a log message (at whatever level you feel appropriate)?
| @@ -348,6 +382,8 @@ def run(self, inputExposure, refObjLoader=None, dataId=None): | |||
| dataId : `dict` or `lsst.daf.butler.DataCoordinate` | |||
| The dataId of the exposure (and detector) bright stars should be | |||
| extracted from. | |||
| skyCorr : `lsst.afw.math.backgroundList.BackgroundList` | |||
| Sky correction. | |||
There was a problem hiding this comment.
Could be slightly more descriptive?
| @@ -348,6 +382,8 @@ def run(self, inputExposure, refObjLoader=None, dataId=None): | |||
| dataId : `dict` or `lsst.daf.butler.DataCoordinate` | |||
| The dataId of the exposure (and detector) bright stars should be | |||
| extracted from. | |||
| skyCorr : `lsst.afw.math.backgroundList.BackgroundList` | |||
There was a problem hiding this comment.
...or None, optional
https://developer.lsst.io/python/numpydoc.html#optional-parameters
| @@ -0,0 +1,187 @@ | |||
| # This file is part of pipe_tasks. | |||
There was a problem hiding this comment.
I'm confused about the addition of this file in the context of this ticket?
There was a problem hiding this comment.
Oh yeah, I guess that's just me failing hard at git somehow. Removed it. Sorry!
| @@ -134,6 +145,11 @@ class ProcessBrightStarsConfig(pipeBase.PipelineTaskConfig, | |||
| " annular flux.", | |||
| default=('BAD', 'CR', 'CROSSTALK', 'EDGE', 'NO_DATA', 'SAT', 'SUSPECT', 'UNMASKEDNAN') | |||
| ) | |||
| doApplySkyCorr = pexConfig.Field( | |||
| dtype=bool, | |||
| doc="Apply sky correction before extracting stars?", | |||
There was a problem hiding this comment.
Can you be a bit more specific about which "sky correction" you are referring to here (even just adding "full focal plane" would help...there...are...just...so...many...!!!)?
There was a problem hiding this comment.
Your update snuck into the bug fix commit 😉
MorganSchmitz
force-pushed
the
tickets/DM-28600
branch
2 times, most recently
from
ed1d70f
to
60deade
Compare
MorganSchmitz
force-pushed
the
tickets/DM-28600
branch
from
60deade
to
3a63c2d
Compare
| @@ -134,6 +145,11 @@ class ProcessBrightStarsConfig(pipeBase.PipelineTaskConfig, | |||
| " annular flux.", | |||
| default=('BAD', 'CR', 'CROSSTALK', 'EDGE', 'NO_DATA', 'SAT', 'SUSPECT', 'UNMASKEDNAN') | |||
| ) | |||
| doApplySkyCorr = pexConfig.Field( | |||
| dtype=bool, | |||
| doc="Apply sky correction before extracting stars?", | |||
There was a problem hiding this comment.
Your update snuck into the bug fix commit 😉
| code extends over the entire focal plane, this can produce | ||
| better sky subtraction. | ||
| The calexp is updated in-place. | ||
| Parameters |
There was a problem hiding this comment.
Still need a space after title description.
| self.modelStampSize = (int(self.config.stampSize[0]*self.config.modelStampBuffer), | ||
| int(self.config.stampSize[1]*self.config.modelStampBuffer)) | ||
| self.modelStampSize = [int(self.config.stampSize[0]*self.config.modelStampBuffer), | ||
| int(self.config.stampSize[1]*self.config.modelStampBuffer)] |
There was a problem hiding this comment.
I’m fine with this bug fix here and would keep it as it’s own commit, but could you split out the unrelated doc update and add a note in the commit message describing the bug that needed fixing?
The modelStampSize attribute of ProcessBrightStarsTask was created as a tuple. This led to an error when the stamp size was even on a side, as a check is immediately made and the entries are changed to ensure an odd stamp size. This commit makes modelStampSize a list instead.
MorganSchmitz
force-pushed
the
tickets/DM-28600
branch
from
3a63c2d
to
8b7839b
Compare
Jira ticket
Jenkins run #33511