DM-23026: Make sure ic source fields are being propagated #334
Conversation
| @@ -124,7 +139,7 @@ class ProcessCcdWithFakesConfig(PipelineTaskConfig, | |||
| doc="The apply aperture correction task to use.") | |||
|
|
|||
| catalogCalculation = pexConfig.ConfigurableField(target=CatalogCalculationTask, | |||
| doc="The catalog calculation ask to use.") | |||
| doc="The catalog calculation task to use.") | |||
There was a problem hiding this comment.
This belongs in a separate commit.
| def __init__(self, schema=None, **kwargs): | ||
| """Initalize tings! This should go above in the class docstring | ||
| def __init__(self, schema=None, butler=None, **kwargs): | ||
| """Initalize things! This should go above in the class docstring |
There was a problem hiding this comment.
These two lines belong in separate commits.
|
|
||
| resultStruct = pipeBase.Struct(outputExposure=exposure, outputCat=sourceCat) | ||
| return resultStruct | ||
|
|
||
| def copyIcSourceFields(self, icSourceCat, sourceCat): |
There was a problem hiding this comment.
Can you use a more descriptive name? In particular, Ic doesn't mean much unless you know the history of the butler. How about copyCalibrationColumns?
Similarly, I think this would read better if the variable names were changed to be more generic, e.g., fromCat and toCat, or calibCat and srcCat.
| The fields copied are those specified by `config.icSourceFieldsToCopy` that actually exist | ||
| in the schema of `icSourceCat`. | ||
|
|
||
| This version was based on and adapted from the one in calibrateTask. |
There was a problem hiding this comment.
How much code is shared? Would it be possible to factor it out of both places?
I've also wondered if it's worth making this operation a Task in its own right, to allow more extensive re-use.
| numUniqueSources = len(set(m[1].getId() for m in matches)) | ||
| if numUniqueSources != numMatches: | ||
| self.log.warn("{} icSourceCat sources matched only {} sourceCat " | ||
| "sources".format(numMatches, numUniqueSources)) |
There was a problem hiding this comment.
For logging, you should not format the string yourself, so this should be:
self.log.warn("%d icSourceCat sources matched only %d sourceCat sources", numMatches, numUniqueSources)
| schemaItem = icSourceCat.schema.find(fieldName) | ||
| except KeyError: | ||
| missingFieldNames.append(fieldName) | ||
| else: |
There was a problem hiding this comment.
This is prolly a matter of personal preference, but since everything you're doing is embedded in a for loop, I'd put a continue in the except block, get rid of the else and move its contents to the left --- I'd find that easier to read. Totally up to you.
| missingFieldNames = [] | ||
| for fieldName in self.config.icSourceFieldsToCopy: | ||
| try: | ||
| schemaItem = icSourceCat.schema.find(fieldName) |
There was a problem hiding this comment.
Can't you just do an if fieldName in icSourceCat.schema?
There was a problem hiding this comment.
I changed this to an if else, which also affects the above comment.
|
|
||
| schema = icSourceSchemaMapper.getOutputSchema() | ||
| newCat = afwTable.SourceCatalog(schema) | ||
| newCat.reserve(len(sourceCat)) |
There was a problem hiding this comment.
It's prolly not necessary (I think extend does it for you), but glad to see you're thinking about memory allocation issues.
| matches = [m for m in matches if m[1].get(deblendKey) == 0] | ||
|
|
||
| # Because we had to allow multiple matches to handle parents, we now | ||
| # need to prune to the best matches |
There was a problem hiding this comment.
Why not prune the catalog before matching?
select = newCat["deblend_nChild"] == 0
matches = afwTable.matchXy(newCat[select], icSourceCat, self.config.matchRadiusPix)
That's a more efficient operation (fewer sources in the catalog) and less code.
There was a problem hiding this comment.
Because I'm silly and didn't really think about what I had copied. Thank you.
| try: | ||
| icSrc.setFootprint(src.getFootprint()) | ||
| src.assign(icSrc, icSourceSchemaMapper) | ||
| finally: |
sr525
force-pushed
the
tickets/DM-23026
branch
2 times, most recently
from
91eeccd
to
69d6b3c
Compare
sr525
force-pushed
the
tickets/DM-23026
branch
from
69d6b3c
to
1d699c8
Compare
…insertion.
The fields detailing which sources were being used as part of some calibration
steps were not being propagated to the fake source catalogs. These are needed
for QA plots and contain useful information. This commit adds functionality to
match between the source catalog generated during processCcdWithFakes and the
icSource catalog that contains the calibration information. The catalogs are
matched and copied into the ouput catalog.
sr525
force-pushed
the
tickets/DM-23026
branch
from
1d699c8
to
64204ac
Compare
No description provided.