DM-23238: Add per row overscan removal #125

plazas · 2020-02-05T17:11:53Z

No description provided.

mfisherlevine

General comment beyond the individual ones though: I am kind of surprised this isn't just a few lines put in around L513 where it says
elif fitType in ('MEDIAN',): if overscanIsInt:
because at that point, you've got the int image already. Can you not just loop over the rows there? Am I misunderstanding the flow here?

tests/test_overscanCorrection.py

mfisherlevine · 2020-02-06T18:20:26Z

tests/test_overscanCorrection.py

@@ -106,7 +111,6 @@ def checkPolyOverscanCorrectionX(self, **kwargs):

        dataBox = lsst.geom.Box2I(lsst.geom.Point2I(0, 0), lsst.geom.Extent2I(10, 10))
        dataImage = afwImage.MaskedImageF(maskedImage, dataBox)
-


Not sure why this empty line (and the one below, and above at L333) got deleted/added, but I'm not really one who cares about this. Others certainly are, so in general try not to do this unless there's a reason for it.

I don't remember why that's there, probably I was testing some code, or put a "print" statement and then deleted it. I don't care either; why is it important?

I think it's just generally untidy and making diffing more confusing. @timj are there reasons to care deeply about such things?

Usually removing or adding lines in parts of the code not being changed is not seen as worthwhile unless it's making the code compliant with some standard. I tend to avoid these sorts of changes because they distract people reading the diff into wondering why the line disappeared. If removing/adding the blank line is wanted it should be done as a distinct commit making clear this is a formatting cleanup.

I also would prefer keeping the blank lines as I find they make the code easier to read. And below on L118 as well. And above on L85.

python/lsst/ip/isr/isrFunctions.py

mfisherlevine · 2020-02-07T00:33:16Z

python/lsst/ip/isr/isrFunctions.py

+            fitTypeStats = afwMath.stringToStatisticsProperty('MEDIAN')
+            # Loop over rows because there is no afwMath.Median(array, axis=1)
+            for row in biasMaskedArr:
+                if row.mask is False:


I need to go in with a debugger, but is that ever True? Isn't row.mask the mask array for a whole row? If so, that never is False.

Actually, one of the unit tests found that it could be False if this condition is not satisfied: https://github.com/lsst/ip_isr/blob/tickets/DM-23238/python/lsst/ip/isr/isrFunctions.py#L545 (a masked numpy array is not created).

It might == False but surely it can't be False, right?

i.e. remember that 0 == False is True, but 0 is False is not.

You have to be careful if a mask is in theory a numpy array here. There are many definitions of "truth" available depending on whether there is any numpy mask attached and whether it's filled with zeroes. If mask is meant to be a numpy array I would not expect it to ever be truly the boolean False. Is this really testing that mask is None rather than False?

mfisherlevine · 2020-02-07T00:33:37Z

python/lsst/ip/isr/isrFunctions.py

+            # Loop over rows because there is no afwMath.Median(array, axis=1)
+            for row in biasMaskedArr:
+                if row.mask is False:
+                    row.mask = numpy.repeat(False, len(row))


And even then, I'm not quite sure what this is doing really.

The second argument of medianRow = afwMath.makeStatistics(row, row.mask, fitTypeStats, statControl).getValue()---one line below---is expecting a list. It fails if it just False, so I'm creating a list of False to pass a valid argument.

I don't think you have to pass in a mask at all. Can't you just remove the argument and call with a different signature?

Also, as discussed OOB, I don't think we ever want to mask a row - it makes sense for things where a spline or poly is being fitted up the column, but for a row-wise median we have to have a number for every row.

python/lsst/ip/isr/isrFunctions.py

tests/test_isrTask.py

mfisherlevine · 2020-02-07T01:35:08Z

python/lsst/ip/isr/isrFunctions.py

+            for row in biasMaskedArr:
+                if row.mask is False:
+                    row.mask = numpy.repeat(False, len(row))
+                medianRow = afwMath.makeStatistics(row, row.mask, fitTypeStats, statControl).getValue()


Also, I just realised, doing this here means you're actually doing a single-round-sigma-clipped row-wise median, right? Not saying that's bad per se but trying to understand what's happening here.

There are a few lines before the loop that do some clipping, and then, in the loop, the median is taken, as you point out. This section of the code was originally intended for the polynomial cases of the overscan correction, so it might be redundant in the case of the median per row. But then if I move the loop to the other section, things might break as we pointed out in another comment.

Can you check with @RobertLuptonTheGood (or he can confirm here, now that he's tagged) that he's OK with doing a single round of clipping before doing the per-row median please? I don't it's necessarily bad, but maybe there is some statistical subtlety that will cause him to dislike this.

mfisherlevine · 2020-02-07T18:29:36Z

For any onlookers, @czwa is taking over the review now.

plazas · 2020-02-11T14:48:47Z

I moved the loop that calculates the median per row to the part of the code where it says elif fitType in ('MEDIAN',): if overscanIsInt: (before, it was in the fitType in ('POLY', 'CHEB', 'LEG', 'NATURAL_SPLINE', 'CUBIC_SPLINE', 'AKIMA_SPLINE') part), as initially suggested by Merlin. This part does not have masking or clipping.

czwa

Looks good. I think there are just style issues here.

czwa · 2020-02-10T19:15:14Z

tests/test_overscanCorrection.py

@@ -106,7 +111,6 @@ def checkPolyOverscanCorrectionX(self, **kwargs):

        dataBox = lsst.geom.Box2I(lsst.geom.Point2I(0, 0), lsst.geom.Extent2I(10, 10))
        dataImage = afwImage.MaskedImageF(maskedImage, dataBox)
-


I also would prefer keeping the blank lines as I find they make the code easier to read. And below on L118 as well. And above on L85.

czwa · 2020-02-11T18:46:25Z

python/lsst/ip/isr/isrFunctions.py

+                offArray = offImage.getArray()
+                ovFitArray = overscanFit.getArray()
+            offArray[:, :] = collapsedReshaped
+            ovFitArray[:, :] = numpy.repeat(collapsed, biasArray.shape[1]).reshape(biasArray.shape)


Is it possible to refactor this to use the simpler offArray[:, :] = vector[:, numpy.newaxis] or vector[numpy.newaxis, :] as is done for the spline/polynomial fits below (L629 on master)? I think all these transposes and repeats are correct, but I find the newaxis notation easier to follow.

Yes. I followed this suggestion (thanks), and have followed what is done in the spline/polynomial part.

czwa · 2020-02-11T18:48:35Z

python/lsst/ip/isr/isrFunctions.py


        if overscanIsInt:
            del overscanImageI
-    elif fitType in ('POLY', 'CHEB', 'LEG', 'NATURAL_SPLINE', 'CUBIC_SPLINE', 'AKIMA_SPLINE'):
+    elif fitType in ('POLY', 'CHEB', 'LEG', 'NATURAL_SPLINE', 'CUBIC_SPLINE',


Is this change needed? Same with whitespace below on L561.

I fixed these.

plazas requested a review from mfisherlevine February 5, 2020 17:12

mfisherlevine reviewed Feb 7, 2020

View reviewed changes

czwa approved these changes Feb 11, 2020

View reviewed changes

Add MEDIAN_PER_ROW overscan removal

3d1d6db

plazas force-pushed the tickets/DM-23238 branch from cc75af8 to 3d1d6db Compare February 13, 2020 22:34

plazas merged commit 3d1d6db into master Feb 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-23238: Add per row overscan removal #125

DM-23238: Add per row overscan removal #125

plazas commented Feb 5, 2020

mfisherlevine left a comment •

edited

mfisherlevine Feb 6, 2020

plazas Feb 7, 2020

mfisherlevine Feb 7, 2020

timj Feb 7, 2020

czwa Feb 10, 2020

mfisherlevine Feb 7, 2020

plazas Feb 7, 2020

mfisherlevine Feb 7, 2020

mfisherlevine Feb 7, 2020

timj Feb 7, 2020

mfisherlevine Feb 7, 2020

plazas Feb 7, 2020

mfisherlevine Feb 7, 2020

mfisherlevine Feb 7, 2020

mfisherlevine Feb 7, 2020

plazas Feb 7, 2020

mfisherlevine Feb 7, 2020

mfisherlevine commented Feb 7, 2020

plazas commented Feb 11, 2020

czwa left a comment

czwa Feb 10, 2020

czwa Feb 11, 2020

plazas Feb 12, 2020

czwa Feb 11, 2020

plazas Feb 12, 2020

		@@ -106,7 +111,6 @@ def checkPolyOverscanCorrectionX(self, **kwargs):

		dataBox = lsst.geom.Box2I(lsst.geom.Point2I(0, 0), lsst.geom.Extent2I(10, 10))
		dataImage = afwImage.MaskedImageF(maskedImage, dataBox)

DM-23238: Add per row overscan removal #125

DM-23238: Add per row overscan removal #125

Conversation

plazas commented Feb 5, 2020

mfisherlevine left a comment • edited

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

mfisherlevine commented Feb 7, 2020

plazas commented Feb 11, 2020

czwa 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

mfisherlevine left a comment •

edited