Review for DM-5427: Handle empty variance array gracefuly. #34
Conversation
|
Typos in commit message: "gracefuly", "pixedls". |
| measRecord.set(self.varValue, medVar) | ||
| else: | ||
| measRecord.set(self.varValue, numpy.NaN) | ||
| raise bl.MeasurementError("Footprint empty, or all pixels are masked, can't compute median", bl.FlagHandler.FAILURE) |
There was a problem hiding this comment.
Would it be appropriate to define a specific flag for this failure mode?
Is the reference to bl.FlagHandler.FAILURE appropriate? In some sense, I guess it is, but I think if we were doing it properly we'd actually have to define FAILURE_BAD_CENTROID above as bl.FlagHandler.FAILURE+1 rather than just assuming it's 1; if we can assume it's 1, then maybe we can just assume this is 0. Maybe a specific reference to FlagHandler.FAILURE is helpful to the reader? (On the other hand, I just wasted 5 minutes wondering about it...)
|
In addition to the minor comments on the code itself, please add an appropriate unit test. |
AstroVPK
force-pushed
the
tickets/DM-5427
branch
4 times, most recently
from
75222ef
to
1362528
Compare
| #has all masked pixels at this point. | ||
| self.assertTrue(np.isnan(source.get("base_Variance_value"))) | ||
| self.assertTrue(source.get("base_Variance_flag_emptyFootprint")) | ||
|
|
There was a problem hiding this comment.
If it were possible, it would be nice to make this a separate test method -- it gives us more granularity when detecting failures. You could put all the logic which sets up and runs the test in common function and then have two tests which call it.
SingleFrameVariancePlugin attempts to compute the median of the variance array for pixels in the heavy footprint of a source. Some pixels are ignored because mask flags are set indicating that the pixel is not to be trusted. Sometimes, this results in all pixels getting rejected at which point numpy raises a RuntimeWarning stating that it is trying to compute the median of an empty array. We check for the empty array and raise a MeasurementError if all the pixels are flagged. The testVariance unittest has been modified to test the new behavior.
No description provided.