DM-16235: Jointcal PhotoCalib returns negative mean calibrations #111

parejkoj · 2018-10-24T16:20:49Z

No description provided.

kfindeisen

I feel like you've sacrificed code quality (encapsulation, clear purpose for methods, etc.) in trying to include the negativity checks as soon as possible. Most of my individual suggestions should be quick fixes.

kfindeisen · 2018-10-25T17:32:35Z

include/lsst/jointcal/AstrometryModel.h

@@ -61,6 +62,9 @@ class AstrometryModel {

    virtual ~AstrometryModel(){};

+    /// Return true if this is a "reasonable" model.
+    bool validate(CcdImageList const &ccdImageList) const { return true; }


Kind of a vague spec. What counts as "reasonable"? Physical plausibility? No degenerate terms?

Also, please document ccdImageList; from the description it sounds like validate should not need any parameters.

I assume validate has a default implementation so that overriding methods can call it? If so, it might be worth documenting as a must/should...

Thanks, I've clarified the docstrings.

Sadly, PhotometryModel and AstrometryModel don't actually share a parent class, though they probably should (so that this API can be defined up there). AstrometryModel.validate() exists to fulfill that API, but I need to think through what it should be in practice. I've filed a ticket for the implementation: it would almost certainly be implemented in AstrometryModel itself, not in a child class.

Oh wait, validate can't be overridden. I've been working in Python too long. 🙄

kfindeisen · 2018-10-25T17:44:17Z

include/lsst/jointcal/MeasuredStar.h

+    MeasuredStar(double xx, double yy, double flux, double fluxErr, double instFlux, double instFluxErr)
+            : MeasuredStar(BaseStar(xx, yy, flux, fluxErr)) {
+        setInstFluxAndErr(instFlux, instFluxErr);
+    }


I really don't like this constructor; its six consecutive double parameters seem more than a little error-prone.

Why not call MeasuredStar(BaseStar, double, double)? It's less ambiguous (though four parameters is pushing it even when they are of different types), assuming there's a natural reason why instFlux does not belong in a BaseStar.

I've deleted this constructor: turns out I don't really need it.

BaseStar is any kind of star (Measured, Fitted, Reference), and only the former has instFlux (since it's from a SourceCatalog measurement).

kfindeisen · 2018-10-25T17:47:13Z

include/lsst/jointcal/PhotometryModel.h

@@ -134,6 +136,17 @@ class PhotometryModel {
    /// Dump the contents of the transfos, for debugging.
    virtual void dump(std::ostream &stream = std::cout) const = 0;

+    /// Return true if this is a "reasonable" model.
+    bool validate(CcdImageList const &ccdImageList) const;


Same questions as for AstrometryModel::validate.

kfindeisen · 2018-10-25T17:50:07Z

include/lsst/jointcal/PhotometryModel.h

@@ -165,6 +178,9 @@ class PhotometryModel {

    // Pedestal on flux/magnitude error (percent of flux or delta magnitude)
    double errorPedestal;
+
+    /// lsst.logging instance, to be created by a subclass so that messages have consistent name.
+    LOG_LOGGER _log;


Kind of a brittle implementation, because a subclass author needs to remember to initialize this in all constructors. Why not have an abstract getLogger() method, and let subclasses decide how to implement it (a logger field, a static logger inside the method, etc.)?

Or, if you want a shared public field, why not make the logger a mandatory parameter for the PhotometryModel constructor? That would also guarantee it's correctly initialized.

This is the only comment that I'm not entirely sure what to do with. I agree with your point about brittleness, but the other versions are either more work for the subclasses (e.g. abstract getLogger(), or more work for the caller (construction parameter). Unless I'm missing something about how to implement getLogger()?

Ok, think I came up with a better solution using the constructor. Do you mind taking a quick look at it to see if it's what you meant? Should be the last commit.

kfindeisen · 2018-10-25T17:56:21Z

python/lsst/jointcal/jointcal.py

@@ -585,6 +585,17 @@ def _check_star_lists(self, associations, name):
        if associations.refStarListSize() == 0:
            raise RuntimeError('No stars in the {} reference star list!'.format(name))

+    def _logChi2AndValidate(self, associations, fit, model, chi2Label="Model"):
+        """Compute chi2, log it, validate the model, and return chi2."""


So, this is a pretty blatant violation of the "do one thing" rule. I can understand moving the logger and finiteness tests in here (though logging is not normally considered part of a function/program's spec, it just happens), but why can't you have a separate call for validating the model?

The amount of boilerplate around each call to fit.minimize() during initialization was getting ridiculous and hard to keep uniform, so I flattened it all into one place. There are a number of different calls to minimize() and they each need to be logged, checked, and validated.

I've added a note to DM-8046, which is a perfect place to make this all better.

kfindeisen · 2018-10-25T18:22:34Z

src/PhotometryModel.cc

+    std::vector<double> xRange = {bbox.xMin, bbox.getCenter().x, bbox.xMax};
+    std::vector<double> yRange = {bbox.yMin, bbox.getCenter().y, bbox.yMax};
+    for (auto const &x : xRange)
+        for (auto const &y : yRange) {


Why not put the initializer lists in the loop definition (i.e., get rid of xRange and yRange entirely)? I think it would improve readability.

Because I didn't even think of it. Good idea.

kfindeisen · 2018-10-25T18:24:55Z

src/PhotometryModel.cc

+
+bool PhotometryModel::checkPositiveOnBBox(CcdImage const &ccdImage) const {
+    bool check = true;
+    double result;


Please don't define result until it's used.

kfindeisen · 2018-10-25T18:27:58Z

tests/test_jointcal.py

@@ -39,20 +41,25 @@ def setUp(self):
        self.badChi2.chi2 = 600.0
        self.badChi2.ndof = 100

+        self.nanChi2 = lsst.jointcal.chi2.Chi2Statistic()
+        self.nanChi2.chi2 = np.nan
+        self.nanChi2.ndof = 100


I'm surprised you can't just initialize Chi2Statistic to the value you want (especially since degrees of freedom seems like something that shouldn't change).

Their primary usage is to be incremented during fitting, so they initialize to zero. I'm just making a fake one here (could Mock it, but I don't think it's worth it in this case).

kfindeisen · 2018-10-25T18:29:02Z

tests/test_jointcal.py

                                          self.maxSteps, self.name, self.whatToFit)
        self.assertEqual(chi2, self.badChi2)
-        log.info.assert_called_with("Fit completed with: %s", str(self.badChi2))
+        log.info.assert_called_with("%s %s", "Fit completed", str(self.badChi2))


Given that logging is something that happens on the side, should you really be testing for it?

Do we really assert that "logging happens on the side" in the stack? It's the only way to output some information (like whether something completed successfully, in this case).

The assumption behind logging (and frameworks like log4* specifically) is that the program is running with little to no human oversight, possibly continuously, and that logging statements are only there to provide information that might be useful for reviewing things after the fact (especially if something went wrong). In other words, it should be possible to remove all logging statements and the program will still be correct.

That said, I don't know of any Stack programs that are supposed to produce text output -- we're very much operating in the non-interactive, machine-readable regime.

kfindeisen · 2018-10-25T18:31:12Z

tests/test_photometryModel.py

@@ -170,7 +170,7 @@ def setUp(self):
        self.visitOrder = 3
        self.focalPlaneBBox = self.camera.getFpBBox()
        # tweak to get more than just a constant field for the second ccdImage
-        self.delta = np.arange(20, dtype=float)*-0.2 + 1
+        self.delta = (np.arange(20, dtype=float)*-0.2 + 1)[::-1]


Why are you reversing the order of delta (if that's what this syntax is)?

Added a comment: it makes the lowest order terms largest (which is what a realistic model would have).

kfindeisen · 2018-10-26T18:13:39Z

include/lsst/jointcal/ConstrainedPhotometryModel.h

-        _log = LOG_GET("jointcal.ConstrainedFluxModel");
+            : ConstrainedPhotometryModel(ccdImageList, focalPlaneBBox,
+                                         LOG_GET("jointcal.ConstrainedFluxModel"), visitOrder,
+                                         errorPedestal_) {


Yes, this is exactly what I had in mind... though I underestimated the number of classes whose constructor signatures would need to be changed. 😞

Cleanup ConstrainedPhotometryModel docstring

Previously some of the test PhotometryModels were negative on all of their bbox, because I hadn't thought through what a "correct" PhotoCalibMean should be (answer: less than one!). Adding this method showed that error, which I've fixed.

Validate models after each fit step. AstrometryModel doesn't have any "validation" conditions yet, but we can probably think of some later. Cleanup the steps that happen after `fitter.minimize()`, so that we're always logging chi2 and testing the same things. Add getModel() for AstrometryFit/PhotometryFit (all the more reason to want a common base class).

parejkoj force-pushed the tickets/DM-16235 branch 8 times, most recently from 252f484 to bd7dd61 Compare October 25, 2018 06:42

kfindeisen approved these changes Oct 25, 2018

View reviewed changes

parejkoj force-pushed the tickets/DM-16235 branch 2 times, most recently from 8a14595 to 47d23d2 Compare October 26, 2018 16:04

kfindeisen reviewed Oct 26, 2018

View reviewed changes

parejkoj added 6 commits October 26, 2018 16:19

Add logger interface for PhotometryModel

a595583

Cleanup ConstrainedPhotometryModel docstring

Add checkPositiveOnBBox to PhotometryModel.cc

38f87da

Previously some of the test PhotometryModels were negative on all of their bbox, because I hadn't thought through what a "correct" PhotoCalibMean should be (answer: less than one!). Adding this method showed that error, which I've fixed.

Add docstrings

7e52060

Use original calibration for mean

5495fbc

Cleanup fitter docstring

983d7e4

parejkoj force-pushed the tickets/DM-16235 branch from 74accd8 to 983d7e4 Compare October 26, 2018 21:20

parejkoj merged commit 17d57d5 into master Oct 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-16235: Jointcal PhotoCalib returns negative mean calibrations #111

DM-16235: Jointcal PhotoCalib returns negative mean calibrations #111

parejkoj commented Oct 24, 2018

kfindeisen left a comment

kfindeisen Oct 25, 2018

kfindeisen Oct 25, 2018

parejkoj Oct 26, 2018

kfindeisen Oct 26, 2018

kfindeisen Oct 25, 2018

parejkoj Oct 26, 2018

kfindeisen Oct 25, 2018

kfindeisen Oct 25, 2018

parejkoj Oct 26, 2018

parejkoj Oct 26, 2018

kfindeisen Oct 25, 2018

parejkoj Oct 26, 2018

kfindeisen Oct 25, 2018

parejkoj Oct 26, 2018

kfindeisen Oct 25, 2018

kfindeisen Oct 25, 2018

parejkoj Oct 26, 2018

kfindeisen Oct 25, 2018

parejkoj Oct 26, 2018

kfindeisen Oct 26, 2018 •

edited

kfindeisen Oct 25, 2018

parejkoj Oct 26, 2018

kfindeisen Oct 26, 2018

DM-16235: Jointcal PhotoCalib returns negative mean calibrations #111

DM-16235: Jointcal PhotoCalib returns negative mean calibrations #111

Conversation

parejkoj commented Oct 24, 2018

kfindeisen 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

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

kfindeisen Oct 26, 2018 • 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

kfindeisen Oct 26, 2018 •

edited