DM-52541: Add new SPIKE mask plane #1166
Conversation
isullivan
force-pushed
the
tickets/DM-52541
branch
4 times, most recently
from
52035ea to
aeef4a3
Compare
isullivan
changed the title
| ) | ||
| doMaskDiffractionSpikes = pexConfig.Field( | ||
| dtype=bool, | ||
| default=True, |
There was a problem hiding this comment.
Does this make sense to default to True? Do we need an override then in all non-LSSTCam obs packages to turn this off?
There was a problem hiding this comment.
These settings are correct for ComCam as well, and the task is useful for HSC and DECam. Those instruments ideally would have tweaked magSlope and magOffset parameters, but the task should be run for them.
| dtype=float, | ||
| default=60., | ||
| doc="Margin outside the exposure bounding box to include bright " | ||
| "sources. In arcseconds.", |
There was a problem hiding this comment.
All of these the quotes should line with quotes.
| default="phot_g_mean", | ||
| doc="Fallback flux field in the reference catalog to use for sources" | ||
| " that don't have measurements in the science image's band." | ||
| "The default uses Gaia g-band.", |
There was a problem hiding this comment.
I don't think that's what this config means. Also don't put the default in the doc string! Also also it's Gaia G not gaia g.
There was a problem hiding this comment.
looking at the code below, I guess that's what this is doing ... even though that's not what this config means in other contexts. I would recommend renaming this config.
There was a problem hiding this comment.
I'll change it to fallbackFluxField to remove the false similarity to other contexts.
| default=10, | ||
| doc="Ratio of the length of a diffraction spike to it's width in the" | ||
| " core of the star.", | ||
| ) |
There was a problem hiding this comment.
Do any of these vary per band? Is that something that may be added in the future?
There was a problem hiding this comment.
Bruno's analysis did not show a dependence on band, so we decided to keep this simple for now. I would like to update the calculations to be based on counts in the future, so that it would properly scale with exposure time.
| Returns | ||
| ------- | ||
| brightCat : `lsst.afw.table.SourceCatalog` |
| Returns | ||
| ------- | ||
| brightCat : `lsst.afw.table.SourceCatalog` | ||
| The entries from the reference catalog selected as bright stars. |
There was a problem hiding this comment.
-> stars with diffraction spikes.
| ------- | ||
| brightCat : `lsst.afw.table.SourceCatalog` | ||
| The entries from the reference catalog selected as bright stars. | ||
| """ |
There was a problem hiding this comment.
Does this need to check if self.refObjLoader is not None and specifically raise otherwise (must be initialized or set...)
There was a problem hiding this comment.
There is a check, but it was not the first line. I'll move it up so the check runs before any other code.
| self.config.magnitudeThreshold) | ||
| self.maskSources(xvals, yvals, radii[bright], mask) | ||
| else: | ||
| self.log.info("No bright stars found in the reference catalog; not masking bright sources.") |
There was a problem hiding this comment.
bright -> spike here. Maybe not everywhere, but at least "no masking diffraction spikes"
| radii = [10**(self.config.magSlope*magnitude + self.config.magOffset) for magnitude in magnitudes] | ||
| return np.array(radii) | ||
|
|
||
| def extractMagnitudes(self, refCat, filterLabel): |
There was a problem hiding this comment.
I hate that this code needs to be duplicated from elsewhere, but I'm not sure what else to do.
| nBright = np.count_nonzero(bright) | ||
|
|
||
| mask = exposure.mask | ||
| mask.addMaskPlane(self.config.spikeMask) |
There was a problem hiding this comment.
This is only run if it's on, right? If this is off do other tasks downstream crash (e.g. in the detectAndMeasure with HSC). Or do we trust that this should be the default for everyone? That seems ... surprising.
There was a problem hiding this comment.
Downstream tasks should currently be fine whether this task is run or not, and the SPIKE mask is only added if it is.
isullivan
force-pushed
the
tickets/DM-52541
branch
from
aeef4a3 to
c150136
Compare
| dtype=float, | ||
| default=15, | ||
| doc="Threshold magnitude for treating a star from the reference catalog" | ||
| " as bright.", |
isullivan
force-pushed
the
tickets/DM-52541
branch
2 times, most recently
from
39ad916 to
28ccb3d
Compare
isullivan
force-pushed
the
tickets/DM-52541
branch
2 times, most recently
from
2e71603 to
c2ad7d1
Compare
Bright sources off the image will also have diffraction spikes calculated and masked if they overlap the image.
isullivan
force-pushed
the
tickets/DM-52541
branch
from
c2ad7d1 to
63948bd
Compare
Adds a new subtask to calibrateImage that masks regions containing diffraction spikes near bright stars drawn from the photometric reference catalog.