DM-12445: Set appropriate default configs for CompareWarp Coadds #162

yalsayyad · 2017-11-16T18:55:17Z

No description provided.

PaulPrice

Some changes required, but shouldn't be too hard.

PaulPrice · 2017-11-16T20:36:04Z

python/lsst/pipe/tasks/assembleCoadd.py

-            "Otherwise, do not mask.",
-        dtype=float,
-        default=0.075
+    maxNumEpochs = pexConfig.Field(


I would have thought that a fraction would be so much more generally useful than a hard number. I'm worried that a hard number isn't going to scale as you go from n=4 to n=400 inputs.

Just made a new ticket for this: DM-12692. I had talked briefly with @RobertLuptonTheGood about it. You're right maxNumEpochs won't scale, but neither will a fraction of the maximum number of epochs. I've seen some steep Nepoch gradients in a patch. Its also depends on how many repeat pointings line up the artifacts.

Yeah, it probably needs some combination of a number and a fraction, maybe max(maxNumEpochs, fracMaxEpochs*num)?

PaulPrice · 2017-11-16T20:49:28Z

python/lsst/pipe/tasks/assembleCoadd.py

@@ -1769,25 +1766,25 @@ def computeAltMaskList(self, tempExpRefList, maskSpanSets):

        return altMaskList

-    def _filterArtifacts(self, spanSetList, epochCountImage, maxNumEpochs=None):
+    def _filterArtifacts(self, spanSetList, epochCountImage):


This method needs a docstring.

PaulPrice · 2017-11-16T20:49:37Z

python/lsst/pipe/tasks/assembleCoadd.py

        maskSpanSetList = []
        x0, y0 = epochCountImage.getXY0()
        for i, span in enumerate(spanSetList):
            y, x = span.indices()
            counts = epochCountImage.array[[y1 - y0 for y1 in y], [x1 - x0 for x1 in x]]
-            idx = numpy.where((counts > 0) & (counts <= maxNumEpochs))
+            idx = numpy.where((counts > 0) & (counts <= self.config.maxNumEpochs))
            percentBelowThreshold = len(idx[0]) / len(counts)
            if percentBelowThreshold > self.config.spatialThreshold:
                maskSpanSetList.append(span)
        return maskSpanSetList

    def _readAndComputeWarpDiff(self, warpRef, imageScaler, templateCoadd):


This method needs a docstring.

PaulPrice · 2017-11-16T20:51:54Z

python/lsst/pipe/tasks/assembleCoadd.py

        maskSpanSetList = []
        x0, y0 = epochCountImage.getXY0()
        for i, span in enumerate(spanSetList):
            y, x = span.indices()
            counts = epochCountImage.array[[y1 - y0 for y1 in y], [x1 - x0 for x1 in x]]
-            idx = numpy.where((counts > 0) & (counts <= maxNumEpochs))
+            idx = numpy.where((counts > 0) & (counts <= self.config.maxNumEpochs))


You should avoid using numpy.where whenever you can (it's expensive), and just use a boolean array. Then, len(idx[0]) below simply becomes array.sum().

PaulPrice · 2017-11-16T20:56:28Z

python/lsst/pipe/tasks/assembleCoadd.py

-        # Warp comparison must use PSF-Matched Warps regardless of requested coadd warp type
-        warpName = self.getTempExpDatasetName('psfMatched')
+        # Warp comparison should use PSF-Matched Warps regardless of requested coadd warp type
+        warpName = self.getTempExpDatasetName(self.config.assembleStaticSkyModel.warpType)


It's not a good idea to go reaching through multiple config elements to get what you want: what if I retarget the assembleStaticSkyModel and name warpType something else? You should only access what you control. That suggests putting warpType into the config at this level, and passing it down to the assembleStaticSkyModel as it needs it.

Hmmm. How would you have done this? Passed the new config down to the subtask in validate? I configure the defaults for assembleStaticSkyModel subtask (an AssembleCoaddTask instance) in the CompareWarpAssembleConfig's setDefaults, already requiring that it have a warpType. It's going to be 'psfMatched' in every scenario I can think of (but then again I lack imagination). I'm debating if its worth making a new config parameter or just leaving it hardcoded.

We could just leave it hardcoded for now: I also lack the imagination to think as to why anyone would use a different warpType, since the algorithm for rejecting artifacts requires PSF-matched inputs.
Or put warpType into the AssembleCoaddTask's Config, and then pass it to the AssembleStaticSkyModelTask constructor when you makeSubtask.

I thought configs frozen by the time we got to making the subtask, but I'll give it try.

The config is definitely frozen within CompareWarpAssembleCoadd.__init__

(Pdb) kwargs['config'].assembleStaticSkyModel.warpType = staticSkyModelWarpType *** FieldValidationError: ConfigurableField 'assembleStaticSkyModel' failed validation: Cannot modify a frozen Config For more information read the Field definition at: File pex/config/configurableField.py:271 (__deepcopy__)And the Config definition at: File pipe/tasks/assembleCoadd.py:1439 (<module>)

To clarify: AssembleStaticSkyModelTask is not a thing. It is a vanilla AssembleCoaddTask used as a subtask within CompareWarpAssembleCoaddTask.

Only solution I can think of at the moment is:

def validate(self): AssembleCoaddConfig.validate(self) self.assembleStaticSkyModel.warpType = self.staticSkyModelWarpType

which I'm not sure is any better because it smells like code duplication. staticSkyModelWarpType is just a copied and pasted warpType config from:
https://github.com/lsst/pipe_tasks/blob/master/python/lsst/pipe/tasks/assembleCoadd.py#L55

Reverting to hardcoded 'psfMatched'. Jenkins running.

PaulPrice · 2017-11-16T20:58:01Z

python/lsst/pipe/tasks/assembleCoadd.py

        doc="Unitless fraction of pixels defining how much of the outlier region has to meet the "
            "temporal criteria",
        dtype=float,
-        default=0.5
+        default=0.5,
+        min=0., max=1.


Add inclusiveMax=True?

PaulPrice · 2017-11-16T20:58:39Z

python/lsst/pipe/tasks/assembleCoadd.py

        self.assembleStaticSkyModel.warpType = 'psfMatched'
-        self.assembleStaticSkyModel.statistic = 'MEDIAN'
+        self.assembleStaticSkyModel.statistic = 'MEANCLIP'
+        self.assembleStaticSkyModel.sigmaClip = 1.5


Why such harsh clipping?

Borrowed from SafeClip. Running a grid of configs is still a to do: DM-12698

But harsh clipping like this is part of the SafeClip algorithm (we deliberately toss out anything that looks even remotely bad in order to make a very clean model of the sky), whereas I wouldn't have thought it would be for this.

This is the same idea. Make a very very clean and naive robust PSF-match coadd to serve as the naive model of the static sky.

Ah, I see --- this is for the assembleStaticSkyModel. Fair enough, then.

For 2< N<=5 especially, we have to be strict to get the junk out of the naive template:

import lsst.afw.math as afwMath arr = [1,10,10,20] sctrl = afwMath.StatisticsControl() sctrl.setNumSigmaClip(1.5) sctrl.setNumIter(3) print afwMath.makeStatistics(arr, afwMath.MEANCLIP, sctrl).getValue() print afwMath.makeStatistics(arr, afwMath.STDEV, sctrl).getValue() > 10.0 > 7.76208734813

Perhaps that's due to a shortcoming in our statistics suite (no robust stdev), or because you're not using STDEVCLIP:

>>> ss = afwMath.makeStatistics(arr, afwMath.MEANCLIP | afwMath.STDEVCLIP | afwMath.NCLIPPED, sctrl) >>> ss.getValue(afwMath.MEANCLIP) 10.0 >>> ss.getValue(afwMath.STDEVCLIP) 0.0 >>> ss.getValue(afwMath.NCLIPPED) 2.0

(NCLIPPED is a feature being implemented on DM-9953.)

That's cool. My point was that if I increase sctrl.setNumSigmaClip(1.5) to sctrl.setNumSigmaClip(2.6) the 2 outliers don't get clipped anymore.

>>> sctrl.setNumSigmaClip(2.6) >>> print afwMath.makeStatistics(arr, afwMath.MEANCLIP, sctrl).getValue() 10.25

OK. How common is it that 40% of the inputs have outliers?

Replace config parameter that controls maximum number of epochs an artifact candidate can appear in without being masking, from a fraction of N to an integer.

PaulPrice changed the title ~~Tickets/dm 12445~~ DM-12445: Set appropriate default configs for CompareWarp Coadds Nov 16, 2017

PaulPrice approved these changes Nov 16, 2017

View reviewed changes

yalsayyad force-pushed the tickets/DM-12445 branch 3 times, most recently from 9b0b32e to d0a5edd Compare November 18, 2017 00:44

yalsayyad added 3 commits November 17, 2017 20:31

Replace temporalThreshold with maxNumEpochs

7e191fa

Replace config parameter that controls maximum number of epochs an artifact candidate can appear in without being masking, from a fraction of N to an integer.

Add a docstring and remove a duplicate function call

eea84d2

Set CompareWarp configuration defaults

db3de1f

yalsayyad force-pushed the tickets/DM-12445 branch from d0a5edd to db3de1f Compare November 18, 2017 02:32

yalsayyad merged commit db3de1f into master Nov 18, 2017

ktlim deleted the tickets/DM-12445 branch August 25, 2018 06:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-12445: Set appropriate default configs for CompareWarp Coadds #162

DM-12445: Set appropriate default configs for CompareWarp Coadds #162

yalsayyad commented Nov 16, 2017

PaulPrice left a comment

PaulPrice Nov 16, 2017

yalsayyad Nov 17, 2017

PaulPrice Nov 17, 2017

PaulPrice Nov 16, 2017

PaulPrice Nov 16, 2017

PaulPrice Nov 16, 2017

PaulPrice Nov 16, 2017

yalsayyad Nov 17, 2017

PaulPrice Nov 17, 2017

yalsayyad Nov 17, 2017

yalsayyad Nov 18, 2017

yalsayyad Nov 18, 2017

PaulPrice Nov 16, 2017

PaulPrice Nov 16, 2017

yalsayyad Nov 17, 2017

PaulPrice Nov 17, 2017

yalsayyad Nov 17, 2017

PaulPrice Nov 17, 2017

yalsayyad Nov 17, 2017

PaulPrice Nov 17, 2017

yalsayyad Nov 17, 2017

PaulPrice Nov 17, 2017

DM-12445: Set appropriate default configs for CompareWarp Coadds #162

DM-12445: Set appropriate default configs for CompareWarp Coadds #162

Conversation

yalsayyad commented Nov 16, 2017

PaulPrice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment