DM-15241 log error on large chi2 #103
Conversation
|
|
||
| # log a message for a large final chi2, TODO: DM-15247 for something better | ||
| if chi2.chi2/chi2.ndof >= 4.0: | ||
| self.log.error("Potentially bad fit: High chi-squared/ndof.") |
There was a problem hiding this comment.
Also print the value so the user can decide for themselves if the fit is bad or not? Would this be more appropriate as a warning or do you definitely want it to be an error?
There was a problem hiding this comment.
The value itself is printed by the log.info line just above this, so no need to repeat it.
I think an error is correct for now: a high chi2 suggests there is something wrong with the fit. In the future, we'll probably want to have two different values, one with a warning and one with an error, but I'd rather be loud for now, while we're learning what the possible failure modes are.
|
|
||
| class TestJointcalIterateFit(lsst.utils.tests.TestCase): | ||
| def setUp(self): | ||
| struct = lsst.jointcal.testUtils.createTwoFakeCcdImages(100, 100) |
There was a problem hiding this comment.
This is a delightful name for this particular testUtil 💯
tests/test_jointcal.py
Outdated
| self.jointcal = lsst.jointcal.JointcalTask(config=self.config) | ||
|
|
||
| self.goodChi2 = lsst.jointcal.chi2.Chi2Statistic() | ||
| self.goodChi2.chi2 = 200.0 |
There was a problem hiding this comment.
Statistically speaking, shouldn't a "good" reduced chi2 be close to 1, not close to 2?
There was a problem hiding this comment.
Yes, but most of the photometry fits are coming in around 2, likely because we don't have a color correction term, and possibly because the model is not complex enough. I'll add a comment though.
| self.fitter.computeChi2.return_value = self.goodChi2 | ||
| self.fitter.minimize.return_value = MinimizeResult.Converged | ||
| self.associations = unittest.mock.Mock(spec=lsst.jointcal.Associations) | ||
| self.associations.getCcdImageList.return_value = self.ccdImageList |
There was a problem hiding this comment.
I think I've managed to follow what is happening here, but a couple of comments might help future users spend less time staring at it than I did. Something like "Create a mock photometry fitter and a mock astrometry fitter / Set the desired return values for a successful jointcal fit"
tests/test_jointcal.py
Outdated
| self.associations.getCcdImageList.return_value = self.ccdImageList | ||
|
|
||
| def test_iterateFit_success(self): | ||
| chi2 = self.jointcal._iterate_fit(self.associations, self.fitter, 20, "testing", "") |
There was a problem hiding this comment.
For these and subsequent lines where you feed a function self.associations, self.fitter, and other stuff, can you include the arg names so it's clearer what things like "20" and "testing" and "random blank entry" are for?
tests/test_jointcal.py
Outdated
|
|
||
| chi2 = self.jointcal._iterate_fit(self.associations, self.fitter, 20, "testing", "") | ||
| self.assertEqual(chi2, self.badChi2) | ||
| log.error.assert_called_with("Potentially bad fit: High chi-squared/ndof.") |
There was a problem hiding this comment.
I've added a test that checks whether the such a log message is emitted.
tests/test_jointcal.py
Outdated
| self.fitter.minimize.return_value = MinimizeResult.Failed | ||
|
|
||
| with self.assertRaises(RuntimeError): | ||
| self.jointcal._iterate_fit(self.associations, self.fitter, 20, "testing", "") |
There was a problem hiding this comment.
How does this bit work? You tell python to raise a RuntimeError and then do some stuff? And because it fails, the call_count is only equal to 1 rather than 2?
There was a problem hiding this comment.
I set minimize.return_value to a value that should cause _iterate_fit to raise. If minimize fails, we raise immediately and there's no "final" minimize call (jointcal.py:782, if you care), so only one call to minimize.
tests/test_jointcal.py
Outdated
| chi2 = self.jointcal._iterate_fit(self.associations, self.fitter, maxSteps, "testing", "") | ||
| self.assertEqual(chi2, self.goodChi2) | ||
| self.assertEqual(self.fitter.minimize.call_count, maxSteps) | ||
| log.error.assert_called_with("testing failed to converge after 3 steps") |
There was a problem hiding this comment.
Use maxSteps variable in the mock log message
tests/test_jointcal.py
Outdated
| log.error.assert_called_with("Potentially bad fit: High chi-squared/ndof.") | ||
|
|
||
| def test_iterateFit_exceedMaxSteps(self): | ||
| log = unittest.mock.Mock(spec=lsst.log.Log) |
There was a problem hiding this comment.
I understand why you make a mock jointcal associations and fitter above, but what purpose does a mock log serve? It doesn't look like you do much of anything with it.
There was a problem hiding this comment.
Having the mock log lets me check whether the logger was called with particular values, instead of trying to either capture stdout and scan it for a particular log message, or interrogate the lsst.log object itself (and I'm not even sure that's possible).
parejkoj
force-pushed
the
tickets/DM-15241
branch
from
70238d5
to
ffa2480
Compare
parejkoj
force-pushed
the
tickets/DM-15241
branch
from
ffa2480
to
b7464c8
Compare
No description provided.