DM-22221: Add subtask to find and mask satellite trails #370

cmsaunders · 2020-04-07T15:11:53Z

This commit adds a new subtask in findSatellites.py and a call to
that task in CompareWarpAssembleCoaddTask, where it is used to
detect satellite trails or other linear features in difference
images. This will only run if the config option
doFilterMorphological is set to True.

yalsayyad

Just the unit test

yalsayyad · 2020-04-09T00:04:08Z

tests/test_findSatellites.py

+
+    def setUp(self):
+        self.fst = FindSatellitesTask()
+        self.fst.config.dChi2Tolerance = 1e-10


We don't change the state of a config after the Task has been instantiated. In fact, when a cmdlineTask is launched it calls Config.freeze() to make config to make it immutable: https://github.com/lsst/pipe_base/blob/master/python/lsst/pipe/base/argumentParser.py#L725 This isn't enforced when using as a regular task, which is why this works. But the standard way of doing what you want to do is to edit the config and then instantiate the Task

self.config = FindSatellitesTask.ConfigClass() self.config.dChi2Tolerance = 1e-10 self.fst = FindSatellitesTask(config=self.config)

*In this example I'm saving the config as an instance variable because you'll reuse it later in line 43

yalsayyad · 2020-04-09T00:12:51Z

tests/test_findSatellites.py

+        self.testy = 600
+        self.exposure = lsst.afw.image.ExposureF(self.testy, self.testx)
+        self.exposure.maskedImage.image.array = np.random.randn(self.testx, self.testy).astype(np.float32)
+        self.exposure.maskedImage.variance.array = np.ones((self.testx, self.testy)).astype(np.float32)


self.exposure.variance.set(1)

yalsayyad · 2020-04-09T00:26:24Z

tests/test_findSatellites.py

+        self.testx = 500
+        self.testy = 600
+        self.exposure = lsst.afw.image.ExposureF(self.testy, self.testx)
+        self.exposure.maskedImage.image.array = np.random.randn(self.testx, self.testy).astype(np.float32)


A stack-y way of doing this is:

rand = lsst.afw.math.Random() lsst.afw.math.randomGaussianImage(exposure.image, rand)

which also has the benefit of making your test deterministic, because there is a default seed in lsst.afw.math.random.Random(algorithm: str, seed: int=1)

yalsayyad · 2020-04-09T00:35:09Z

tests/test_findSatellites.py

+
+        reshapeBinning = self.fst.processMaskedImage(self.exposure)
+        with self.assertWarns(Warning):
+            scipyBinning = self.fst.processMaskedImage(self.exposure, checkMethod=True)


By calling this parameter checkMethod you are communicating that this option only exist to support these unit tests? Is that true? By reading your inline comments in the task, I'd think that it's a slower, more flexible option.

yalsayyad · 2020-04-09T00:48:00Z

tests/test_findSatellites.py

@@ -0,0 +1,94 @@
+import unittest


Need the preamble. See https://developer.lsst.io/stack/license-and-copyright.html?#python-preamble on how to get one from @sqrbot-jr

yalsayyad · 2020-04-09T00:57:27Z

tests/test_findSatellites.py

+            scipyBinning = self.fst.processMaskedImage(self.exposure, checkMethod=True)
+        self.assertAlmostEqual(reshapeBinning.tolist(), scipyBinning.tolist())
+
+        self.fst.config.imageBinning = None


People look to unit tests as examples, since they're guaranteed to work. Whereas this works, it sets a bad example. Please instantiate a new task rather than modifying a config.

self.config.imageBinning = None task = FindSatelliteTask(config=self.config) nobinImage = task.processMaskedImage(self.exposure)

yalsayyad · 2020-04-09T01:26:42Z

tests/test_findSatellites.py

+
+        result = self.fst.run(testExposure, binaryImage=binaryImage)
+        self.assertEqual(len(result.lines), 1)
+        self.assertLess(abs(input['rho'] - result.lines[0]['rho']), 0.01)


self.assertAlmostEqual(input['rho'], result.lines[0]['rho'], delta=0.01) would prob be more readable,

yalsayyad · 2020-04-09T02:46:35Z

tests/test_findSatellites.py

+
+        # Make image with line and check that input line is recovered:
+        testExposure = self.exposure.clone()
+        input = np.array((150, 45, 3), dtype=[('rho', float), ('theta', float), ('sigma', float)])


input is a builtin. https://developer.lsst.io/python/style.html?#user-defined-names-should-not-shadow-python-built-in-functions

tests/test_findSatellites.py

natelust

I'm going to leave these comments now, so everyone can look over them. I may read over this again soon to see if there is anything else I see.

natelust · 2020-04-08T13:37:47Z

python/lsst/pipe/tasks/findSatellites.py

+#
+import lsst.pex.config as pexConfig
+import lsst.pipe.base as pipeBase
+import lsst.newhoughtransform


In general a space between lsst imports and standard imports is nice

Is lsst.newhoughtransform a new LSST supported package that wraps the new third party hough transform package?

RFC-680. This code is just undergoing first round review while that RFC is implemented.

lsst.newhoughtransform is a placeholder until we get the third-party hough transform package implemented, but the functionality should be the same.

natelust · 2020-04-08T13:38:48Z

python/lsst/pipe/tasks/findSatellites.py

+from sklearn.cluster import KMeans
+import warnings
+
+__all__ = ["FindSatellitesConfig", "FindSatellitesTask"]


I know we are not great at following this convention, but I think all should be above the imports

Indeed on both counts: https://developer.lsst.io/python/style.html?#standard-code-order-should-be-followed

natelust · 2020-04-08T13:39:33Z

python/lsst/pipe/tasks/findSatellites.py

+    a line following the given coordinates.
+    """
+    def __init__(self, data, weights, mask, invSigmaInit, line=None):
+        """


its weird, but the parameters documentation of init should be with the class level documentation not in init

natelust · 2020-04-08T13:40:02Z

python/lsst/pipe/tasks/findSatellites.py

+            2d array with mask
+        invSigmaInit : float
+            Initial guess for inverse of Moffat sigma parameter
+        line : np.ndarray (optional)


indicate somewhere that it will default to None

Change the data type to reflect what it really is, a numpy.recarray.

This part of the comment is a little program design heavy, and you might know it already, but if not I hope it helps. I would strongly suggest you consider using a different data structure that you define. Record arrays can be useful, but often they are an anti pattern. If you created a simple data structure using namedtuple (we talked about this in bookclub) or a dataclass (we will talk about this soon in book club) it would make it easier for other people (or future you) to work with this code.

Data structures you create will have their own types which would make function signatures easier to understand, i.e. know exactly what needs to be passed. These data structures have their own schema that people will be able to see, and will be required to populate when the object is created.

For instance if you had a datastructure like

# There is a version of this using typing.NamedTuple, but that involves type annotations # that we have not talked about yet, so using what book club covered. LineOutline = namedtuple("LineOutline", ("rho", "theta", "invSigma"))

you could be sure that the line argument passed to this function would always has theta defined. This is also useful for the user, because they know what "schema" the object might have. Because recarrays are basically arbitrary containers, a user would have no idea without reading your code either here or where one is produced what this object should have in it.

Defined datastructures also have the advantage that you can check their type if you really wanted to validate that something is exactly the type you want. They are also more reusable in other locations, and provide a single place where you can change a definition rather than relying on identical implementations of recarray creation at multiple places in a code base. This will make refactoring code later much easier.

Ok long comment over, this is an optional suggestion, but I hope you find it useful to think about.

I have implemented a Line dataclass and a LineSet class as discussed. These should remove all of the cases where there are recarrays or separate variables for rho, theta, and sigma, so this also addresses a lot of other comments on findSatellites.py.

natelust · 2020-04-08T13:44:03Z

python/lsst/pipe/tasks/findSatellites.py

+        yrange = np.arange(ymax) - ymax / 2.
+        self.rhoMax = ((0.5 * ymax)**2 + (0.5 * xmax)**2)**0.5
+        self.xmesh, self.ymesh = np.meshgrid(xrange, yrange)
+        self.mask = (weights != 0)


The () are not needed here

I just do this because I think it is more readable. I will remove the parentheses if it is against the convention.

natelust · 2020-04-13T15:29:38Z

python/lsst/pipe/tasks/findSatellites.py

+            # change more than the allowed bin in rho or theta:
+            if ((abs(fit.rho - line.rho) > 2 * self.config.rhoBin) or
+               (abs(fit.theta - line.theta) > 2 * self.config.thetaBin)):
+                fitSuccess = False


I would probably restructure this if statement as

if not fitSuccess or abs(fit.rho - line.rho) > 2 * self.config.rhoBin or abs(fit.theta - line.theta) > 2 * self.config.thetaBin: continue

and drop the second conditional below

(ignore the weird formatting in the above, use standard pep8 spacing)

fitSuccess originally comes from a few lines further up. I wanted to only have on continue statement for all fit failures, so that is why I have formatted it this way, if that makes sense.

natelust · 2020-04-13T15:41:45Z

python/lsst/pipe/tasks/findSatellites.py

+            lineModel.setLineMask(fit)
+            finalModel = lineModel.makeProfile(fit.rho, fit.theta, fit.invSigma)
+            # Take absolute value, as trails are allowed to be negative
+            finalModelMax = (abs(finalModel)).max()


you should not need the extra ()

natelust · 2020-04-13T15:42:42Z

python/lsst/pipe/tasks/findSatellites.py

+            lineFits.append([fit.rho, fit.theta, sigma, chi2, finalModelMax])
+            finalLineMasks.append(finalLineMask)
+
+        if len(lineFits) == 0:


See comment about about a container class for all of this. The constructor of that type can be setup such that you would not need the if else in this case.

natelust · 2020-04-13T18:52:48Z

python/lsst/pipe/tasks/findSatellites.py

+        line : np.ndarray (optional)
+            Guess for position of line, with datatype names `rho`, `theta`, and
+            `sigma`
+        """


When you build a class it is important to think about what story, its interface, that you want told to the rest of the world. If a user gets passed an instance of your class what are they supposed to know about it, and how should they use it. This is part of documentation yes, but also in what methods/members your class presents to them. Should a user look at an instance of your class and do instance.sigma to get sigma info? Is instance.mask how they should be getting the mask, or should it be the result of a method call? Anything that would be an implementation detail should have its name start with an underscore.

For instance would a user ever call setLineMask on some instance they were passed, or is this a method you used in your class while doing some calculation the user does care about. If the later, the name should be _setLineMask. Same is true for the attributes. I am pretty sure users should not be looking at initLine so it should be _initLine.

Setting up your names like this does a few things.

it provides a smaller api that you are reasonably sure will not change, so you only have to keep a guarantee on those interfaces, and you are free to refactor the rest without fearing you will break something someone was using.

It is self documenting, as in when a user does dir they are presented with only the interfaces that are important for them, helping them in discoverability

It helps other developers know what you intended when you wrote the code. If someone comes along and sees 2 public methods that can be sure those are the meat and potatoes and to focus on them when developing against your library, or writing a new compatible one.

Im sure if you talked to others there would be more things on this list, but these are the ones that sprang to mind now. In LSST we have not been particularly good about this in the past, but it would be really great if people coming up to speed started doing this more, so that our newest code, that others are likely to see and use will already be cleaner and more maintainable.

natelust · 2020-04-13T19:10:17Z

python/lsst/pipe/tasks/assembleCoadd.py

+                    fitResult = self.findSatellites.run(warpDiffExp, slateIm)
+                    satMask = afwImage.Mask(coaddBBox)
+                    satMask.array[fitResult.mask] = 1
+                    spanSetSatellite = afwGeom.SpanSet.fromMask(satMask)


I am pretty sure what you want to do here is to call the split method on the spanset that is returned from fromMask. fromMask returns one giant SpanSet that basically just maps exactly onto the mask. But I think what you want here is a bunch of individual SpanSets with all the rest dropped. This is basically a bunch of footprints of all the satellites that are to be ignored. @yalsayyad may know better than me about what this algorithm expects though.

Also, if possible, could your findSatellites.run just use afwMasks? Or return one maybe?

It probably doesn't matter if we treat one satellite at a time or all-the-satellites as one.... This does explain why list.append(list) below worked...Because it wasn't a list!

Unrelated, can you move this doFilterMorphological block above the doPrefilterArtifacts block. I think I want to prefilter artifacts contained by your satellites too eventually.

For moving the doFilterMorphological block above the doPrefilterArtifacts block, there are a couple issues. First, the satellite code uses slateIm, which is created from the prefiltered artifacts. I think interpolated regions are prefiltered here (as SUSPECT, but maybe I am wrong), and it is important that these are removed before running findSatellites.
Second, I don't think prefilterArtifacts works well for the satellite trails. I tried what I think you are suggesting when I was figuring out the implementation, but ran into problems. Often the satellite trail intersects with another large footprint, like a bright star. Then, the "bad" fraction of the total footprint ends up being less than self.prefilterArtifactsRatio, so the whole footprint is kept, not filtered out and the satellite trail still goes into the final image.

yalsayyad · 2020-04-13T20:50:41Z

python/lsst/pipe/tasks/findSatellites.py

+            # Initial estimate should be quite close: fit is deemed unsuccessful if rho or theta
+            # change more than the allowed bin in rho or theta:
+            if ((abs(fit.rho - line.rho) > 2 * self.config.rhoBin) or
+               (abs(fit.theta - line.theta) > 2 * self.config.thetaBin)):


E128: indent one more

yalsayyad · 2020-04-14T01:57:34Z

python/lsst/pipe/tasks/assembleCoadd.py

@@ -2232,6 +2256,8 @@ def findArtifacts(self, templateCoadd, tempExpRefList, imageScalerList):
            if spanSetList:
                filteredSpanSetList = self.filterArtifacts(spanSetList, epochCountImage, nImage,
                                                           templateFootprints)
+                if self.config.doFilterMorphological and (spanSetBadMorphoList[i].getArea() != 0):
+                    filteredSpanSetList.append(spanSetBadMorphoList[i])


You mean += instead of append?

>>> a = [1,2,3] >>> b = [4,5,6] >>> a.append(b) >>> a [1, 2, 3, [4, 5, 6]] >>> a = [1,2,3] >>> a += b >>> a [1, 2, 3, 4, 5, 6]

I think this whole block should get its own loop, in the scientifically impossible but programmatically possible chance that there are no artifacts but there is a satellite trail.

if self.config.doFilterMorphological: for i, (satellites, artifacts) in enumerate(zip(spanSetBadMorphoList, spanSetArtifactList)): artifacts += satellites

yalsayyad · 2020-04-14T02:17:58Z

python/lsst/pipe/tasks/assembleCoadd.py

+                    fitResult = self.findSatellites.run(warpDiffExp, slateIm)
+                    satMask = afwImage.Mask(coaddBBox)
+                    satMask.array[fitResult.mask] = 1
+                    spanSetSatellite = afwGeom.SpanSet.fromMask(satMask)


It probably doesn't matter if we treat one satellite at a time or all-the-satellites as one.... This does explain why list.append(list) below worked...Because it wasn't a list!

Unrelated, can you move this doFilterMorphological block above the doPrefilterArtifacts block. I think I want to prefilter artifacts contained by your satellites too eventually.

yalsayyad · 2020-04-14T02:20:56Z

python/lsst/pipe/tasks/findSatellites.py

+    """
+
+    ConfigClass = FindSatellitesConfig
+    _DefaultName = "findSatellitesConfig"


_DefaultName = "findSatellites"

yalsayyad · 2020-04-14T02:23:26Z

python/lsst/pipe/tasks/findSatellites.py

+        dtype=float,
+        default=10.**-1,
+    )
+    imageBinning = pexConfig.Field(


We call this binSize in SubtractBackgroundTask and other parts of the stack. imageBin would keep it consistent with your thetaBin etc.. I'd support rhoBinSize, imageBinSize

yalsayyad · 2020-04-14T02:28:40Z

python/lsst/pipe/tasks/findSatellites.py

+        dtype=float,
+        default=2,
+    )
+    minImageSignaltoNoise = pexConfig.Field(


essentially the same thing as thresholdValue in SourceDetectionTask.
https://github.com/lsst/meas_algorithms/blob/master/python/lsst/meas/algorithms/detection.py#L66 Your processMaskedImage = SourceDetectionTask (with thresholdType='pixel_stdev') + binning.

I still think minImageSignaltoNoise is not going to register as the detection threshold for stack users. And since I just called it the detection threshold without thinking about it, I'd recommend detectionThreshold.

yalsayyad · 2020-04-14T02:29:46Z

python/lsst/pipe/tasks/findSatellites.py

+    imageBinning = pexConfig.Field(
+        doc="Number of pixels by which to bin image",
+        dtype=int,
+        default=2,


Out of curiosity, why bin the image? Does it make the kht faster?

It does make the kht faster, but it also helps make satellite trails sharper.

yalsayyad · 2020-04-14T02:37:00Z

python/lsst/pipe/tasks/findSatellites.py

+    """Configuration parameters for `FindSatellitesTask`
+    """
+    minimumKernelHeight = pexConfig.Field(
+        doc="minimum height of the satellite finding kernel relative to the tallest kernel",


"satellite-finding". Start with a Capital M. Units? When I hear "relative," I think fractional, rather than additive offset.

It is fractional.

yalsayyad · 2020-04-14T02:47:31Z

python/lsst/pipe/tasks/findSatellites.py

+        mask = maskedImage.getMask()
+
+        detectionMask = ((mask.array & mask.getPlaneBitMask("DETECTED")))
+        badMask = ((mask.array & mask.getPlaneBitMask("NO_DATA")) |


For example, that would look like:

badPixelMask = mask.getPlaneBitMask(self.config.badMaskPlanes) badMask = (mask.array & badPixelMask) > 0

badMask is already a bool.

yalsayyad · 2020-04-14T03:03:35Z

python/lsst/pipe/tasks/findSatellites.py

+        fitWeights = np.copy(weights)
+        fitWeights[~fitMask] = 0
+
+        if binning is not None:


In the long run, this isn't the right place for a binning algorithm. Consider opening a ticket to move this lower into the stack. In the stack we have afwMath.binImage() which can only handle means. Most the image binning used by the stack is actually inafwMath.makeBackground which can do clipped means and medians. https://github.com/lsst/afw/blob/master/doc/lsst.afw.math/Background-example.rst

Also, why do you need inverse-variance weighted means when binning this image?

I looked at using afwMath.binImage() here, but I think it would be more convoluted. Even without doing a weighted mean, you need the binned weights in order to get the S/N at the end. I think this means that, to use afwMath.binImage(), you would have make an afwImage ImageF or similar, put the weights in that, and run afwMath.binImage() on that.

Separately, doing a weighted mean takes care of removing bad pixels, though it is probably not crucial otherwise.

yalsayyad

Adding comments I started a while back. 4 comments are from before the module got renamed to maskStreaks. I took a screenshot if they don't show up. We can talk about higher level stuff today.

yalsayyad · 2020-06-16T21:48:20Z

python/lsst/pipe/tasks/findSatellites.py

+        2d array of data
+    weights : np.ndarray
+        2d array of weights
+    mask : np.ndarray


There's no mask argument in the constructor

yalsayyad · 2020-06-16T21:57:25Z

python/lsst/pipe/tasks/findSatellites.py

+        dtype=float,
+        default=0.2,
+    )
+    nSigmas = pexConfig.Field(


nSigma would be easier to remember IMO.

yalsayyad · 2020-06-16T22:05:27Z

python/lsst/pipe/tasks/findSatellites.py

+        dtype=float,
+        default=2,
+    )
+    minImageSignaltoNoise = pexConfig.Field(


I still think minImageSignaltoNoise is not going to register as the detection threshold for stack users. And since I just called it the detection threshold without thinking about it, I'd recommend detectionThreshold.

yalsayyad · 2020-06-16T22:11:15Z

python/lsst/pipe/tasks/findSatellites.py

+        default=("NO_DATA", "INTRP", "BAD", "SAT", "EDGE")
+    )
+    detectedPlanes = pexConfig.ListField(
+        doc="Pixels that were detected above threshold in image",


Would you add what this is used for in the docstring?

yalsayyad · 2020-07-10T04:20:24Z

python/lsst/pipe/tasks/maskStreaks.py

+        )
+
+    def processMaskedImage(self, maskedImage, forceSlowBin=False):
+        """Make binary image array from maskedImage object


This summary string describes the input and output but doesn't really describe what it does which is (I think) "Make a detection map" or "Bin and detect" The stack has various detection tasks and binning routines, and we need to make clear why we need another one and eventually move it to where the others live.

This function has been renamed "setDetectionMask" and downgraded to a helper function outside the class.

yalsayyad · 2020-07-10T04:21:28Z

python/lsst/pipe/tasks/maskStreaks.py

+
+        Parameters
+        ----------
+        maskedImage : `lsst.afw.image.Exposure`


Calling a lsst.afw.image.Exposure maskedImage is confusing.

yalsayyad · 2020-07-10T04:29:45Z

python/lsst/pipe/tasks/maskStreaks.py

+        Returns
+        -------
+        out_data : `np.ndarray`
+            2-d binary image of pixels above the signal-to-noise threshold.


np.zeros makes an array of float64s which is what it returns now, right? Not a great return type for a boolean map.

This has been removed because the function no longer returns anything, it just sets a mask plane in the input image.

yalsayyad · 2020-07-10T04:32:37Z

python/lsst/pipe/tasks/maskStreaks.py

+            ind = binnedDenominator != 0
+            np.divide(binnedNumerator, binnedDenominator, out=binnedData, where=ind)
+            binnedWeight = binnedDenominator
+            binMask = (binnedData * binnedWeight**0.5) > self.config.minImageSignaltoNoise


binMask is a boolean array which you could return as-is

yalsayyad · 2020-07-10T04:35:22Z

python/lsst/pipe/tasks/maskStreaks.py

+
+        Returns
+        -------
+        #lines : `np.ndarray'


Forgot to delete these lines?

yalsayyad · 2020-07-10T04:48:57Z

python/lsst/pipe/tasks/maskStreaks.py

+            - ``originalLines``: lines identified by kernel hough transform
+            - ``lineClusters``:  lines grouped into clusters in rho-theta space
+            - ``lines``: final result for lines after line-profile fit
+            - ``mask``: 2-d boolean mask where detected lines=0


I'd expect lines to be True/1 and empty space to be False/0

natelust · 2020-07-22T14:49:27Z

python/lsst/pipe/tasks/maskStreaks.py

+    sigma: float = 0
+
+
+class LineSet:


This class does not implement the set protocol, so it probably should not be named set. Maybe LineCollection?

natelust · 2020-07-22T14:49:46Z

python/lsst/pipe/tasks/maskStreaks.py

+
+
+class LineSet:
+    """Set of `Line` objects.


If you change the name change this

natelust · 2020-07-22T14:51:52Z

python/lsst/pipe/tasks/maskStreaks.py

+        else:
+            self.sigmas = np.zeros(len(self.rhos))
+
+        self.lines = [Line(rho, theta, sigma) for (rho, theta, sigma) in


Do you want to allow there to be duplicate identical lines in this object?

I dont think you are exposing the fact that there is a lines attribute outside of the class (since you have a __len__ and __getitem__) so you should make this self._lines

natelust · 2020-07-22T14:55:38Z

python/lsst/pipe/tasks/maskStreaks.py

+
+    def __getitem__(self, index):
+        return self.lines[index]
+


You are implementing __len__ and __getitem__ which is enough for iteration, but it would be more efficient if you were to implement __iter__ as well:

def __iter__(self): return iter(self._lines)

natelust · 2020-07-22T15:00:36Z

python/lsst/pipe/tasks/maskStreaks.py

+        return self.lines[index]
+
+    def __repr__(self):
+        return ", ".join(str(line) for line in self.lines)


This could get REALLY long if you print this out, consider wrapping this in a textwrap.shorten call with some width like:

return textwrap.shorten(<joined string>, width=80, placeholder="...")

This will be more friendly to people that print it out

natelust · 2020-07-22T15:04:23Z

python/lsst/pipe/tasks/maskStreaks.py

+        """
+        self.rhos *= scalingFactor
+        self.lines = [Line(rho, theta, sigma) for (rho, theta, sigma) in
+                      zip(self.rhos, self.thetas, self.sigmas)]


Consider adding a rescaleRho to the Line class so you can just say

for line in self._lines: line.rescale(scalingFactor)

This will save you from creating the objects all over again

I removed the need for rescaling here.

natelust · 2020-07-22T15:06:48Z

python/lsst/pipe/tasks/maskStreaks.py

+        newLine : `Line`
+            `Line` to add to current set of lines
+        """
+        self.lines.append(newLine)


Do you want to take ownership of the line you are adding, so if it changes on the outside it does not change inside your class? Or for performance do you want to want to rely on people to be good?

self.lines.append(copy.copy(newLine))

natelust · 2020-07-22T15:25:49Z

python/lsst/pipe/tasks/maskStreaks.py

+        """
+
+        # Renormalize variables so that expected standard deviation in a
+        # cluster is 1.


I'm not sure I get how what you are doing below follows from the comment you have here, but I may just be missing something.

This has been rewritten.

natelust · 2020-07-22T15:28:43Z

python/lsst/pipe/tasks/maskStreaks.py

+            # change more than the allowed bin in rho or theta:
+            if ((abs(fit.rho - line.rho) > 2 * self.config.rhoBinSize)
+                    or (abs(fit.theta - line.theta) > 2 * self.config.thetaBinSize)):
+                fitSuccess = False


If you changed this var name to fitFailed it would be more natural to say

if fitFailed: continue

natelust · 2020-07-22T15:29:20Z

python/lsst/pipe/tasks/maskStreaks.py

+
+        Returns
+        -------
+        #lines : `np.ndarray'


I don't think there should be # here

natelust

A few more minor things, but your almost there!

natelust · 2020-08-07T13:35:04Z

python/lsst/pipe/tasks/maskStreaks.py

+
+    @property
+    def rhos(self):
+        return np.array([line.rho for line in self._lines])


change this to np.array(line.rho for line in self._lines). This will use a generator expression to build the array and saves you from creating an intermediate list that just gets thrown away.

natelust · 2020-08-07T13:35:13Z

python/lsst/pipe/tasks/maskStreaks.py

+
+    @property
+    def thetas(self):
+        return np.array([line.theta for line in self._lines])


same as above

natelust · 2020-08-07T13:38:54Z

python/lsst/pipe/tasks/maskStreaks.py

+        finalModel : np.ndarray
+            Model for line profile
+        """
+        model, dmodel = self._makeMaskedProfile(line, fitFlux=fitFlux)


Im surprised flake8 did not warn you about this. if you are not using dmodel, you should assign it to _ instead:
model, _ = self._makeMaskedProfile(line, fitFlux=fitFlux)

natelust · 2020-08-07T13:46:46Z

python/lsst/pipe/tasks/maskStreaks.py

+            dChi2 = oldChi2 - chi2
+            cholesky = scipy.linalg.cho_factor(A)
+            dx = scipy.linalg.cho_solve(cholesky, b)
+


You could save from defining a new line_search function on each interation of the while if you factored it outside the loop and had it take a second parameter dx and then pass dx in the brent call:

def line_search(c, dx): testx = x - c * dx testLine = Line(testx[0], testx[1], testx[2]**-1) return self._lineChi2(testLine, grad=False) while abs(dChi2) > dChi2Tol: .... factor, fmin, _, _ = scipy.optimize.brent(line_search, args=(dx,), full_output=True, tol=0.05)

natelust · 2020-08-07T13:49:53Z

python/lsst/pipe/tasks/maskStreaks.py

+            - ``mask``: 2-d boolean mask where detected lines are True
+        """
+        mask = maskedImage.getMask()
+        detectionMask = ((mask.array & mask.getPlaneBitMask(self.config.detectedMaskPlane)))


two too many ()

natelust · 2020-08-07T13:51:11Z

python/lsst/pipe/tasks/maskStreaks.py

+        Parameters
+        ----------
+        maskedImage : `lsst.afw.image.maskedImage`
+            The image in which to search for streaks.


maybe make a note that the maskedImage mask detection plane is expected to be populated in a way required by your task

natelust · 2020-08-07T13:53:01Z

python/lsst/pipe/tasks/maskStreaks.py

+        """
+        filterData = image.astype(int)
+        cannyData = canny(filterData, low_threshold=0, high_threshold=1, sigma=0.1)
+        return cannyData


you can totally leave this as is, I just want to highlight you can save lines, typing, and readying by putting

return canny(filterData, low_threshold=0, high_threshold=1, sigma=0.1)

natelust · 2020-08-07T13:56:23Z

python/lsst/pipe/tasks/maskStreaks.py

+        default=2,
+    )
+    invSigma = pexConfig.Field(
+        doc="Moffat sigma parameter (in units of pixels) describing the "


inverse of the sigma parameter?

natelust · 2020-08-07T14:02:50Z

python/lsst/pipe/tasks/maskStreaks.py

+# see <http://www.lsstcorp.org/LegalNotices/>.
+#
+
+__all__ = ["MaskStreaksConfig", "MaskStreaksTask"]


if you want setDetectionMask to be available for other code (like tests) it must be added to __all__, I dont know how the tests were working w/o this.

natelust · 2020-08-07T14:15:56Z

tests/test_maskStreaks.py

+        self.maskName = "STREAK"
+        self.detectedPlane = "DETECTED"
+
+    """def test_input(self):


Something is up with this test, it is commented out? Should you put it back in? Should it be taken out? As an fyi you can use unittest.skip decorator to skip a test while you are testing and debugging. The final test output will indicate that one test was skipped so it is not forgotten like a comment.

No, this test should have been removed. It is no longer relevant, since we don't allow a separate "binary image" anymore.

yalsayyad · 2020-08-07T21:07:47Z

python/lsst/pipe/tasks/maskStreaks.py

+        Defaults to None, in which case only data with `weights` = 0 is masked
+        out.
+    """
+    def __init__(self, data, weights, line=None):


https://developer.lsst.io/python/numpydoc.html#docstrings-of-classes-should-be-followed-but-not-preceded-by-a-blank-line

Class docs are followed by blank line. Method docs are not. My linter picks this up as E301.

yalsayyad · 2020-08-07T21:08:21Z

python/lsst/pipe/tasks/maskStreaks.py

+    sigmas : np.ndarray (optional)
+        Array of `Line` sigma parameters
+    """
+    def __init__(self, rhos, thetas, sigmas=None):


https://developer.lsst.io/python/numpydoc.html#docstrings-of-classes-should-be-followed-but-not-preceded-by-a-blank-line

Class docs are followed by blank line. Method docs are not. My linter picks this up as E301.

yalsayyad · 2020-08-07T21:09:31Z

python/lsst/pipe/tasks/maskStreaks.py

+        give the same result.
+    binning : int (optional)
+        Number of pixels by which to bin image
+    detectedPlanes : str (optional)


detectedPlane vs detectedPlanes

This commit adds a new subtask in findSatellites.py and a call to that task in CompareWarpAssembleCoaddTask, where it is used to detect satellite trails or other linear features in difference images. This will only run if the config option doFilterMorphological is set to True.

yalsayyad reviewed Apr 9, 2020

View reviewed changes

natelust requested changes Apr 13, 2020

View reviewed changes

yalsayyad reviewed Apr 14, 2020

View reviewed changes

cmsaunders force-pushed the tickets/DM-22221 branch from dc722a5 to 74a215f Compare June 4, 2020 19:34

cmsaunders force-pushed the tickets/DM-22221 branch 2 times, most recently from 73584de to cc9a920 Compare June 22, 2020 15:55

yalsayyad reviewed Jul 29, 2020

View reviewed changes

natelust reviewed Jul 29, 2020

View reviewed changes

cmsaunders force-pushed the tickets/DM-22221 branch from cc9a920 to 13674da Compare August 5, 2020 16:08

natelust requested changes Aug 7, 2020

View reviewed changes

yalsayyad reviewed Aug 26, 2020

View reviewed changes

natelust approved these changes Aug 26, 2020

View reviewed changes

cmsaunders force-pushed the tickets/DM-22221 branch from 13674da to 5809d18 Compare August 28, 2020 13:56

cmsaunders merged commit 7fb4cbd into master Aug 28, 2020

DM-22221: Add subtask to find and mask satellite trails #370

DM-22221: Add subtask to find and mask satellite trails #370

Conversation

cmsaunders commented Apr 7, 2020

yalsayyad 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

natelust 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

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

yalsayyad 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment