Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Check that the gain is non-negative when setting the variance #47

Merged
merged 2 commits into from
Feb 2, 2018
Merged

Check that the gain is non-negative when setting the variance #47

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a0451c4
Check that the gain is non-negative when setting the variance
RobertLuptonTheGood Feb 1, 2018
2ffccc9
Added unit test for gain >= 0
RobertLuptonTheGood Feb 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 8 additions & 2 deletions python/lsst/ip/isr/isrTask.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -661,10 +661,16 @@ def updateVariance(self, ampExposure, amp):
@param[in,out] ampExposure exposure to process
@param[in] amp amplifier detector information
"""
if not math.isnan(amp.getGain()):
gain = amp.getGain()
if not math.isnan(gain):
if gain <= 0:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not combine these lines as if not math.isnan(gain) and gain <= 0:?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the action on NaN and <= 0 is different.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, ok, I see it now, outside the changed code block.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there anything further to do on this?

patchedGain = 1.0
self.log.warn("Gain for amp %s == %g <= 0; setting to %f" % (amp.getName(), gain, patchedGain))
gain = patchedGain

isrFunctions.updateVariance(
maskedImage=ampExposure.getMaskedImage(),
gain=amp.getGain(),
gain=gain,
readNoise=amp.getReadNoise(),
)

Expand Down
24 changes: 24 additions & 0 deletions tests/test_flatAndIlluminationCorrection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,30 @@ def testIllum2(self):
def testIllum3(self):
self.doIllum(scaling=3.7)

def testGainAndReadnoise(self):
import lsst.afw.image as afwImage
from lsst.afw.cameraGeom.testUtils import DetectorWrapper
from lsst.ip.isr import IsrTask
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are there imports inside the test method?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no downside, but I'm happy to move them. That's probably the rule anyway (I should know)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. As-is, this will fail flake8 (which you apparently aren't running?).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imports have now been removed from the method and there are no flake8 violations


isrTask = IsrTask()

detector = DetectorWrapper().detector
raw = afwImage.ExposureF(detector.getBBox())

level = 10
readNoise = 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just made aware that readNoise=1 is not helpful, given the definition of variance below...

raw.image.set(level)

amp = detector[0]
amp.setReadNoise(readNoise)

for gain in [-1, 0, 0.1, 1]:
amp.setGain(gain)
isrTask.updateVariance(raw, amp)
if gain <= 0:
gain = 1

self.assertEqual(raw.variance.get(0, 0), level/gain + readNoise**2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. That still doesn't test the nan case.

  2. How do you know the code is treating all the pixels correctly unless you actually test all the pixels?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@leannep leannep Sep 13, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


class MemoryTester(lsst.utils.tests.MemoryTestCase):
pass
Expand Down