Allow configuring more statistical options for assembleCoadd #142
Conversation
| "SUM": "sum", | ||
| "MEANSQUARE": "mean of square of pixel values", | ||
| } | ||
| ) |
There was a problem hiding this comment.
Why do we need to specify these? There's code in Statistics that takes a string and converts it to a valid enum; why don't we use that in the validate method?
Listing statistics here is what caused this problem in the first place.
There was a problem hiding this comment.
Yeah, writing out the allowed options here really sucks, doesn't it. I liked the idea of the options being exposed in the resulting config files. What I really want is the stats class to give me a dictionary of available statistics for stacking. I talked this over with @pschella, who says the new API will offer that. I ended up specifying them explicitly because:
- Not all statistics area available for stacking: NOTHING, ERROR, ORMASK make no sense in a coadd and stackStatistics raises InvalidParameter errors or segfault (in the case of ORMASK)
- The only documentation for the options in the
Property enumis in code comments. There's no way to retrieve them.
Without exposing the options, I could add to the config's validate:
UNSTACKABLE_STATS = ['NOTHING', 'ERROR', 'ORMASK']
if not hasattr(afwMath.Property, self.statistic) or self.statistic in UNSTACKABLE_STATS:
stackableStats = [str(k) for k in afwMath.Property.__members__.keys()
if str(k) not in UNSTACKABLE_STATS]
raise ValueError("statistic %s is not allowed. Please choose one of %s."
% (self.statistic, stackableStats))
sufficient solution?
There was a problem hiding this comment.
It'll print out:
ValueError: statistic ORMASK is not allowed. Please choose one of ['NPOINT', 'ERRORS', 'MIN', 'STDEVCLIP', 'SUM', 'VARIANCECLIP', 'MEDIAN', 'MEANCLIP', 'IQRANGE', 'STDEV', 'VARIANCE', 'MEANSQUARE', 'MAX', 'MEAN'].
| \ref assembleSubregion to mean-stack the corresponding subregions from the coaddTempExps (with outlier | ||
| rejection if config.doSigmaClip=True). Set the edge bits the the coadd mask based on the weight map. | ||
| \ref assembleSubregion to stack the corresponding subregions from the coaddTempExps with the | ||
| statistic specified. Set the edge bits the the coadd mask based on the weight map. |
There was a problem hiding this comment.
"the the" - I know it was there before, but it seems like you can fix it also.
| # Build the unclipped coadd | ||
| coaddMean = AssembleCoaddTask.assemble(self, skyInfo, tempExpRefList, imageScalerList, weightList, | ||
| bgModelList, doClip=False) | ||
| config.statistic = 'MEAN' |
There was a problem hiding this comment.
Haven't you already forced this to be 'MEAN'? Maybe it is just here for clarity?
There was a problem hiding this comment.
It was for clarity and safety in case the validation of statistic==MEAN was removed. I'll add a comment.
Question: How do you feel about forcing the third and final iteration of assemble to use a MEAN? I added that to replicate current behavior. Of course MEAN should be the default, but it doesn't technically need to be enforced anymore.
| @@ -667,7 +691,7 @@ def assembleSubregion(self, coaddExposure, bbox, tempExpRefList, imageScalerList | |||
| matching is enabled, add the background and background variance from each coaddTempExp. Remove mask | |||
| planes listed in config.removeMaskPlanes, Finally, mean-stack | |||
| the actual exposures using \ref afwMath.statisticsStack "statisticsStack" with outlier rejection if | |||
| config.doSigmaClip=True. Assign the stacked subregion back to the coadd. | |||
| config.statistics=MEANCLIP. Assign the stacked subregion back to the coadd. | |||
There was a problem hiding this comment.
Since you have now added more statistics options, I think you need to modify this doc. It assumes you are doing mean or clipping.
yalsayyad
force-pushed
the
tickets/DM-4158
branch
2 times, most recently
from
33d49aa
to
6871fc3
Compare
Specify main stacking statistic in AssembleCoadd and derived tasks with config parameter `statistic`. For backwards compatibility: * Setting the deprecated doSigmaClip to True (now False by default) ignores `statistic` and uses MEANCLIP. * The default statistic is MEANCLIP imitating the previous default of doSigmaClip=True.
yalsayyad
force-pushed
the
tickets/DM-4158
branch
from
6871fc3
to
80f0bf6
Compare
Specify main stacking statistic in AssembleCoadd and derived tasks
with config parameter
statistic. For backwards compatibility:ignores
statisticand uses MEANCLIP.of doSigmaClip=True.