DM-23786: Add bright star subtraction task #555
Conversation
There was a problem hiding this comment.
I've left a ton of comments, but almost all of them are trivial. My major complaint is that there is no test code to run this task. I noticed at least one place where running this task in accordance with the documentation would have still crashed it, so some unit testing is necessary before merging this.
| dimensions=("band",), | ||
| ) | ||
| skyCorr = cT.Input( | ||
| doc="Input Sky Correction to be subtracted from the calexp if doApplySkyCorr=True", |
There was a problem hiding this comment.
doApplySkyCorr should be within `` so it can link to the appropriate variable in the docs
| ) | ||
| inputExtendedPsf = cT.Input( | ||
| doc="Extended PSF model.", | ||
| name="extended_psf", |
There was a problem hiding this comment.
Has it been already defined elsewhere that the name of this input will be extended_psf? While it is within the rules, having some variables in camelCase and some in snake_case is going to get confusing.
There was a problem hiding this comment.
I think this nomenclature has already been established in prior tickets. Future tickets should aim to standardize all the names associated with bright star subtraction.
|
|
||
| def __init__(self, *, config=None): | ||
| super().__init__(config=config) | ||
| if not config.doApplySkyCorr: |
There was a problem hiding this comment.
I am not 100% sure on when the lines in Connections get triggered. Is there a test case that checks that both values of doApplySkyCorr do what we want it to do?
There was a problem hiding this comment.
A future ticket which adds unit testing capability should run this with doApplySkyCorr set to both False and True (i.e., two separate tests).
| "bilinear": "bilinear interpolation", | ||
| "lanczos3": "Lanczos kernel of order 3", | ||
| "lanczos4": "Lanczos kernel of order 4", | ||
| "lanczos5": "Lanczos kernel of order 5", |
There was a problem hiding this comment.
It might be preferable to allow up tolanczos7 as well. In general, the default value being one of the extremes can become inconvenient if we'd like to check the behavior around the default.
There was a problem hiding this comment.
See comment above (added 6 and 7).
| _DefaultName = "subtractBrightStars" | ||
|
|
||
| def __init__(self, initInputs=None, *args, **kwargs): | ||
| pipeBase.CmdLineTask.__init__(self, *args, **kwargs) |
There was a problem hiding this comment.
Can we not user super here?
There was a problem hiding this comment.
Added super(), and updated the inheritance to also include pipeBase.PipelineTask to make this gen3 compatible.
| model : `lsst.afw.image.MaskedImageF` | ||
| The extended PSF model, shifted (and potentially warped) to match | ||
| the bright star's positioning. | ||
| star : `lsst.meas.algorithms.brightStarStamps.BrightStarStamp` |
There was a problem hiding this comment.
Consider using ~lsst.meas.algorithms.brightStarStamps.BrightStarStamp (start with tilde within single backticks). I couldn't find the link in the style guide, but it keeps it short and still points to the right documentation.
There was a problem hiding this comment.
Added - and on relevant line above - and throughout the entire file.
| # "unnormalize" star | ||
| starIm *= star.annularFlux | ||
| # make sure both components use the star mask | ||
| XY = starIm.clone() |
There was a problem hiding this comment.
Variables cannot being with upper case letters, let alone being in all caps unless they are global variables: https://developer.lsst.io/python/style.html#naming-conventions. I also wouldn't rename this to xy because that still doesn't seem very indicative. I was expecting these to be multipled with spatial coordinates, probably to compute an inner product, but that is not what is going on in the code below. Perhaps something like cross and auto correlations?
There was a problem hiding this comment.
Following discussion, agreed to change these to lower case (xx and xy, and their Sum equivalents), and added an in-line comment explaining these variables.
There was a problem hiding this comment.
Or, use xx and xy variables but say the estimator of the scalingFactor (f) that minimizes (Y-fX)^2 is E[XY]/E[XX].
| starIm = afwMath.rotateImageBy90(starIm, nb90Rots) | ||
| # "unnormalize" star | ||
| starIm *= star.annularFlux | ||
| # make sure both components use the star mask |
There was a problem hiding this comment.
More minor quibble - Comments must be full sentences, with starting letter capitalized and ending in a period. This applies to other comments in the file as well.
There was a problem hiding this comment.
All comments in the file modified as suggested. This particular comment (# make sure both components use the star mask) removed, as it seemingly does not apply to this code block.
| inputExtendedPsf : `lsst.pipe.tasks.extended_psf.ExtendedPsf` | ||
| Extended PSF model, produced by | ||
| `lsst.pipe.tasks.extended_psf.MeasureExtendedPsfTask`. | ||
| skyCorr : `lsst.afw.math.backgroundList.BackgroundList` or ``None``, |
There was a problem hiding this comment.
None should be within single backtics.
| to its corresponding input bright star. | ||
| """ | ||
| inputExpBBox = inputExposure.getBBox() | ||
| if self.config.doApplySkyCorr: |
There was a problem hiding this comment.
Do we have to check if skyCorr is not None here (default), or can applySkyCorr handle that case?
There was a problem hiding this comment.
Modified run method doc-string, and added additional skyCorr check for None type.
|
|
||
| class SubtractBrightStarsConnections(pipeBase.PipelineTaskConnections, | ||
| dimensions=("instrument", "visit", "detector"), | ||
| defaultTemplates={"outputExposureName": "subtracted", |
There was a problem hiding this comment.
Thinking long-term about this, is 'subtracted' a good descriptor for these outputs? I.e., many subtractions already take place across the pipeline (and perhaps future additional subtractive data products will also be defined), so perhaps this needs to be more descriptive? bright_star_subtracted_calexp? Although, now that I write it out, this seems too long... I'm not fully convinced either way on this, so I wanted to ping it back to you to hear your thoughts.
There was a problem hiding this comment.
How about brightStar_subtracted_calexp?
There was a problem hiding this comment.
Yes, that sounds good to me - adopted.
| defaultTemplates={"outputExposureName": "subtracted", | ||
| "outputBackgroundName": "brightStars"}): | ||
| inputExposure = cT.Input( | ||
| doc="Input exposure from which to subtract bright star stamps", |
There was a problem hiding this comment.
(very very) minor comment - some of these doc-strings terminate with a period '.', whereas some don't.
| doc="Extended PSF model.", | ||
| name="extended_psf", | ||
| storageClass="ExtendedPsf", | ||
| dimensions=("band",), |
There was a problem hiding this comment.
(another) (very very) minor comment - also, some tuples and/or connection blocks have terminating commas, and some don't.
| doc="Magnitude limit, in Gaia G; all stars brighter than this value will be subtracted", | ||
| default=18 | ||
| ) | ||
| warpingKernelName = pexConfig.ChoiceField( |
There was a problem hiding this comment.
Arun's comment from the other day was to also add lanczos7 here too - I'm not sure how difficult/different/desired this is in practice?
There was a problem hiding this comment.
Added 6 and 7 as allowed options. Future unit testing capability should test that lanczos6 and lanczos7 work as expected.
| doc="Mask planes that, if set, lead to associated pixels not being included in the computation of " | ||
| "the scaling factor. Ignored if scalingType is annularFlux, as the stamps are expected to " | ||
| "already be normalized.", | ||
| default=('BAD', 'CR', 'CROSSTALK', 'EDGE', 'NO_DATA', 'SAT', 'SUSPECT', 'UNMASKEDNAN') |
There was a problem hiding this comment.
Following our discussion yesterday, perhaps a note should be left here (either in the doc-string itself, or otherwise in a code comment block) reminding that secondary DETECTED sources (i.e., all sources other than the primary source) will be added to the BAD mask plane, and therefore, removing BAD from this list will also necessarily remove secondary source masking too.
There was a problem hiding this comment.
Added text to doc string and and in-line comment with more detail.
| This task uses both an extended PSF model produced by | ||
| `lsst.pipe.tasks.extended_psf.MeasureExtendedPsfTask` and a set of bright | ||
| star stamps produced by | ||
| `lsst.pipe.tasks.processBrightStars.ProcessBrightStarsTask`. |
There was a problem hiding this comment.
These two might make more sense being swapped around, to indicate that ProcessBrightStarsTask must be run first, and then MeasureExtendedPsfTask subsequently run on the output.
| An Exposure containing every bright star's profile; its image can | ||
| then be subtracted from the input exposure. |
There was a problem hiding this comment.
Would this be more accurate reading something like An Exposure containing a scaled bright star model fit to every bright star profile; ...? (please re-word as necessary - I just wanted to communicate that the exposure doesn't contain the bright star profile itself, rather, some kind of model fit to said profile).
| # make a copy of the input model | ||
| model = inputExtendedPsf(dataId["detector"]).clone() | ||
| modelStampSize = model.getDimensions() | ||
| inv90Rots = 4 - inputBrightStarStamps.nb90Rots |
There was a problem hiding this comment.
I'm sure this is obvious, but I can't see it right now - why is this 4 here?!
There was a problem hiding this comment.
The 4 minus nb90Rots turns this into an inverse rotation.
There was a problem hiding this comment.
I'm not sure if there's any check to ensure nb90Rots is an integer between 0-3. So this might be better off as
inv90Rots = 4 - inputBrightStarStamps.nb90Rots%4| inv90Rots = 4 - inputBrightStarStamps.nb90Rots | ||
| model = afwMath.rotateImageBy90(model, inv90Rots) | ||
| warpCont = afwMath.WarpingControl(self.config.warpingKernelName) | ||
| invImages = [] |
There was a problem hiding this comment.
Could you add a comment line above this explaining the purpose of the following for loop, e.g.,
# loop over bright stars, computing the inverse transformed and scaled postage stamp for each
leeskelvin
force-pushed
the
tickets/DM-23786
branch
2 times, most recently
from
2eb252a
to
14fa675
Compare
leeskelvin
force-pushed
the
tickets/DM-23786
branch
from
14fa675
to
e7e24ad
Compare
| class SubtractBrightStarsConfig(PipelineTaskConfig, pipelineConnections=SubtractBrightStarsConnections): | ||
| """Configuration parameters for SubtractBrightStarsTask""" | ||
|
|
||
| doWriteSubtractor = Field( |
There was a problem hiding this comment.
Either in this ticket, or in the refactoring ticket that should follow soon, it'd be good to make it as Field[bool] so type annotations can work.
There was a problem hiding this comment.
Type annotations added to all Config options (including ChoiceField, which I think works now?)
| default=("BAD", "CR", "CROSSTALK", "EDGE", "NO_DATA", "SAT", "SUSPECT", "UNMASKEDNAN"), | ||
| ) | ||
| doApplySkyCorr = Field( | ||
| dtype=bool, doc="Apply full focal plane sky correction before extracting stars?", default=True |
There was a problem hiding this comment.
Is this a black thing? I'd like it personally if these were in their own individual lines. One way to make black wrap it would be to give a trailing comma, as is the case in previous Fields.
| def applySkyCorr(self, calexp, skyCorr): | ||
| """Apply correction to the sky background level. | ||
| Sky corrections can be generated with the 'skyCorrection.py' | ||
| executable in pipe_drivers. Because the sky model used by that |
There was a problem hiding this comment.
Is the above line still true?
| xySum = makeStatistics(xy, self.statsFlag, self.statsControl).getValue() | ||
| xxSum = makeStatistics(xx, self.statsFlag, self.statsControl).getValue() | ||
| scalingFactor = xySum / xxSum if xxSum else 0 | ||
| else: |
There was a problem hiding this comment.
Since scalingType is a ChoiceField, an invalid option is caught at config time and not in runtime. So this will never get triggered. You could keep it if you like, but this block is unnecessary IMO.
There was a problem hiding this comment.
Removed. I'd like to replace this with a match during a refactor, as I think that's more readable, but leaving as-is for now sans the final else block.
| # Compute the least squares scaling factor. | ||
| xySum = makeStatistics(xy, self.statsFlag, self.statsControl).getValue() | ||
| xxSum = makeStatistics(xx, self.statsFlag, self.statsControl).getValue() | ||
| scalingFactor = xySum / xxSum if xxSum else 0 |
There was a problem hiding this comment.
Should this default to 1 if xxSum is None or NaN instead of 0? For the default value of inPlace=True, it looks like we might lose the model altogether if scaleModel was called and if makeStatistics behaved improperly.
There was a problem hiding this comment.
Another good catch, changed to 1 👍
| return scalingFactor | ||
|
|
||
| def runQuantum(self, butlerQC, inputRefs, outputRefs): | ||
| """Run a single quantum of the task.""" |
There was a problem hiding this comment.
You can remove this docstring and replace it with a comment "#Docstring inherited." If it didn't have a docstring, it'd inherit it automatically from the parent PipelineTask which is probably sufficient here.
|
|
||
| Parameters | ||
| ---------- | ||
| inputExposure : `afwImage.exposure.exposure.ExposureF` |
There was a problem hiding this comment.
| inputExposure : `afwImage.exposure.exposure.ExposureF` | |
| inputExposure : `~lsst.afw.image.exposure.exposure.ExposureF` |
There was a problem hiding this comment.
Similarly in the Returns section as well.
| # make a copy of the input model | ||
| model = inputExtendedPsf(dataId["detector"]).clone() | ||
| modelStampSize = model.getDimensions() | ||
| inv90Rots = 4 - inputBrightStarStamps.nb90Rots |
There was a problem hiding this comment.
I'm not sure if there's any check to ensure nb90Rots is an integer between 0-3. So this might be better off as
inv90Rots = 4 - inputBrightStarStamps.nb90Rots%4
leeskelvin
force-pushed
the
tickets/DM-23786
branch
from
e7e24ad
to
d58f7dc
Compare
leeskelvin
force-pushed
the
tickets/DM-23786
branch
from
d58f7dc
to
fda16d1
Compare
Jira ticket; Jenkins run #34670