Check that the gain is non-negative when setting the variance #47
Conversation
| if not math.isnan(amp.getGain()): | ||
| gain = amp.getGain() | ||
| if not math.isnan(gain): | ||
| if gain <= 0: |
There was a problem hiding this comment.
Why not combine these lines as if not math.isnan(gain) and gain <= 0:?
There was a problem hiding this comment.
Because the action on NaN and <= 0 is different.
There was a problem hiding this comment.
Oh, ok, I see it now, outside the changed code block.
There was a problem hiding this comment.
is there anything further to do on this?
|
Um, I hadn't approved this yet. I was writing some comments about your unittest... How did you even manage to merge it with a red change marker in place? |
|
Because you asked for a unit test and I added one. I merged and pushed from the command line, as normal.
|
|
I had neither marked this PR as approved, nor marked the original ticket as Reviewed. |
|
Sorry. It was a trivial ticket and I didn’t even think to check that you’d finished signing off. My fault.
… On Feb 2, 2018, at 6:47 PM, John Parejko ***@***.***> wrote:
I had neither marked this PR as approved, nor marked the original ticket as Reviewed.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub <#47 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABOpFXDvFpGhjWtqctILNemvjq6fNmpTks5tQ56HgaJpZM4R2Scr>.
|
| if not math.isnan(amp.getGain()): | ||
| gain = amp.getGain() | ||
| if not math.isnan(gain): | ||
| if gain <= 0: |
There was a problem hiding this comment.
Oh, ok, I see it now, outside the changed code block.
| def testGainAndReadnoise(self): | ||
| import lsst.afw.image as afwImage | ||
| from lsst.afw.cameraGeom.testUtils import DetectorWrapper | ||
| from lsst.ip.isr import IsrTask |
There was a problem hiding this comment.
Why are there imports inside the test method?
There was a problem hiding this comment.
There's no downside, but I'm happy to move them. That's probably the rule anyway (I should know)
There was a problem hiding this comment.
Yes. As-is, this will fail flake8 (which you apparently aren't running?).
There was a problem hiding this comment.
Imports have now been removed from the method and there are no flake8 violations
| if gain <= 0: | ||
| gain = 1 | ||
|
|
||
| self.assertEqual(raw.variance.get(0, 0), level/gain + readNoise**2) |
There was a problem hiding this comment.
I see three possible branches in the code you added: gain is nan, gain <= 0, and gain > 0. There is only one assert here, so it can only test one of those branches.
Separately, why are you doing raw.variance.get(0, 0), instead of checking all values of raw.variance?
There was a problem hiding this comment.
The line's indentation is incorrect; it should be in the loop. I'm not sure how this happened as I checked that the code failed when I reverted the fix. User error.
I only need to check one pixel as they're all the same, so this is simpler and equivalent.
There was a problem hiding this comment.
-
That still doesn't test the nan case.
-
How do you know the code is treating all the pixels correctly unless you actually test all the pixels?
There was a problem hiding this comment.
What NaN case?
I'm adding a test to check that we now handle gains <= 0, so I don't need to test all the pixels -- it wouldn't gain anything.
And I don't understand your question about the readNoise. I'm adding a test that we handle an illegal value from the cameraGeom data, not a test to check that we handle variances correctly. I would like better test coverage for the IsrTask, but that should be calling the IsrTask, not internals.
There was a problem hiding this comment.
The code itself has an if isnan(gain), which means that we need a test case for gain being NaN.
Again, how do you know that all of the pixels are treated correctly unless you test them?
The method you are testing is called updateVariance. There is a term related to readNoise**2. 1 doesn't give you much information when you square it.
There was a problem hiding this comment.
A value 1.5 is now used for read noise. A test for a NaN value of gain has been added. Concerning the question about testing all pixels, I agree with Robert that this is outside the scope of this unit test. This is a unit test for the handling of illegal values from the cameraGeom data, not the correct handling of variances.
| raw = afwImage.ExposureF(detector.getBBox()) | ||
|
|
||
| level = 10 | ||
| readNoise = 1 |
There was a problem hiding this comment.
I was just made aware that readNoise=1 is not helpful, given the definition of variance below...
|
@parejkoj Some of your comments have been address by @RobertLuptonTheGood - could you please resolve the conversations where that is the case. Could you also please summarise what you want to see to merge this PR |
No description provided.