DM-15011 Add ModelVisit/ModelChip options for photometry #96

parejkoj · 2018-07-06T01:25:48Z

No description provided.

kfindeisen

I'm a bit worried about the design implications of this change: specifically, the way ConstrainedPhotometryModel and ChipVisitPhotometryMapping get initialized, and the latter's inheritance relationship with PhotometryMapping, both seem like they could introduce bugs. Minor comments otherwise.

kfindeisen · 2018-07-06T21:18:27Z

python/lsst/jointcal/testUtils.py

@@ -50,6 +50,9 @@ def createTwoFakeCcdImages(num1=4, num2=4, seed=100):
        a square, to have sqrt(num) centroids on a grid.
    seed : `int`, optional
        Seed value for np.random.
+    fakeCcdId : `int`, optional
+        Sensor identifier to use for both CcdImages (they will still represent
+        ccdId=12, as that is the only testdata that is included here).


Sorry, I don't understand this. If I call this function with fakeCcdId=13, will the images "still represent ccdId=12"? What does that mean?

(Also, this function seems like a bit of a violation of the "do one thing" rule...)

I've clarified the text, lets see if that helps.. I agree it's somewhat of a violation of that, but this is all just a way to produce fake (but more complicated than a mock!) datasets to feed to tests of Models.

kfindeisen · 2018-07-06T21:22:30Z

python/lsst/jointcal/testUtils.py

@@ -111,6 +114,9 @@ def createFakeCcdImage(butler, visit, ccdId, num, instFluxKeyName,
        Value to set for calibrationMean in the created PhotoCalib.
    photoCalibErr : `float`, optional
        Value to set for calibrationErr in the created PhotoCalib.
+    fakeCcdId : `int`, optional
+        If set, use this as the ccdId in the returned CcdImage, otherwise
+        use ``ccdId``.


This descriptionis ok, but the description of ccdId itself is kind of confusing. If I set fakeCcdId, what exactly do I need ccdId for (besides "to somehow use with the Butler")?

Good point: it turns out I can remove the ccdId arg completely, so I've done so, and added a few clarifying comments.

kfindeisen · 2018-07-06T21:28:05Z

include/lsst/jointcal/ConstrainedPhotometryModel.h

@@ -77,6 +77,10 @@ class ConstrainedPhotometryModel : public PhotometryModel {
 private:
    PhotometryMappingBase *findMapping(CcdImage const &ccdImage) const override;

+    // Which components of the model are we fitting?
+    bool _fittingChips;
+    bool _fittingVisits;


As a matter of style, you should explicitly initialize these in the constructor(s).

kfindeisen · 2018-07-06T21:38:21Z

src/ConstrainedPhotometryModel.cc

@@ -76,33 +76,56 @@ ConstrainedPhotometryModel::ConstrainedPhotometryModel(CcdImageList const &ccdIm
 }

 unsigned ConstrainedPhotometryModel::assignIndices(std::string const &whatToFit, unsigned firstIndex) {
-    // TODO DM-8046: currently ignoring whatToFit: eventually implement configurability.


Why is this TODO no longer valid? Your code is still stringly typed.

True, but this particular TODO was related to the fact that the existing code ignored whatToFit, which is now fixed.

kfindeisen · 2018-07-06T21:47:08Z

src/ConstrainedPhotometryModel.cc

-        mapping->setIndex(index);
-        index += mapping->getNpar();
+    if (whatToFit.find("Model") == std::string::npos) {
+        LOGLS_ERROR(_log, "assignIndices was called and Model is *not* in whatToFit");


Error, or warning? You are continuing with a no-op. (Should that behavior be documented?)

Good point. I've changed it to a warning and now return the input index. If the code is ever called without "Model", I believe things can continue from that state. This would be made better with some enums instead of string parsing (hence DM-8046).

kfindeisen · 2018-07-06T23:25:46Z

src/PhotometryMapping.cc

@@ -23,30 +23,49 @@ void ChipVisitPhotometryMapping::computeParameterDerivatives(MeasuredStar const

    // NOTE: the chip derivatives start at 0, and the visit derivatives start at the last chip derivative.
    // This is independent of the full-fit indices.
-    Eigen::Ref<Eigen::VectorXd> chipBlock = derivatives.segment(0, _chipMapping->getNpar());


Does the comment still belong here?

Yes, although the code that it applies to now lives inside the two if statements, instead of on the immediately-following lines. Suggestions on how to make that clearer?

What's wrong with

// NOTE: the chip derivatives start at 0. This is independent of the full-fit indices. ... // NOTE: the visit derivatives start at the last chip derivative. This is independent of the full-fit indices.

kfindeisen · 2018-07-06T23:27:30Z

src/PhotometryMapping.cc

-    _visitMapping->getTransfo()->computeParameterDerivatives(measuredStar.getXFocal(),
-                                                             measuredStar.getYFocal(), instFlux, visitBlock);
-    visitBlock *= chipScale;
+    if (_nParVisits > 0) {


No test of _visitMapping->isFixed()?

There is no such thing as a fixed visitMapping in a ChipVisitMapping.

Is it possible that you'd add it in the future but forget to update this line?

Probably not.

kfindeisen · 2018-07-06T23:37:13Z

include/lsst/jointcal/PhotometryMapping.h

@@ -198,7 +198,7 @@ class ChipVisitPhotometryMapping : public PhotometryMappingBase {
              _visitMapping(std::move(visitMapping)) {}


Add explicit initialization of _nParChips and _nParVisits.

kfindeisen · 2018-07-06T23:44:12Z

tests/test_photometryMapping.py

-        self.assertFloatsAlmostEqual(result, expect)
-
+    def _computeChebyshevDerivative(self, star):
+        """Return the derivatives w.r.t. the Chebyshev components."""


Does this factoring follow from the C++-side changes, or should it be a separate commit?

Hmmm... I did it to make test_setWhatToFit easier to write, which is a test of the new C++ code.

Then I would have probably made it a commit before the C++ one. Oh well. :/

kfindeisen · 2018-07-06T23:54:01Z

tests/test_photometryMapping.py

+        self.assertEqual(self.mappingCheby.getNpar(), 0)
+        self.assertEqual(self.mappingCheby.getMappingIndices(), [])
+        result = self.mappingCheby.computeParameterDerivatives(self.star1, self.instFlux)
+        self.assertFloatsEqual(result, np.array([]))


There's a lot of code duplication here. Can you factor it out? It looks like you only need:

which parts of the model are being fit

the set of indices (should you be testing order-sensitive equality?)

the (local) method used to compute the derivative, or (for maximum flexibility) the derivative itself

Good point. I've refactored. And yes, order matters.

kfindeisen · 2018-07-16T19:40:34Z

python/lsst/jointcal/testUtils.py

@@ -111,6 +112,8 @@ def createFakeCcdImage(butler, visit, ccdId, num, instFluxKeyName,
        Value to set for calibrationMean in the created PhotoCalib.
    photoCalibErr : `float`, optional
        Value to set for calibrationErr in the created PhotoCalib.
+    fakeCcdId : `int`, optional
+        If set, use this as the ccdId in the returned CcdImage.


Consider removing the "If set". Presumably the default value of 12 gets used instead of being a magic number for "unspecified"?

kfindeisen · 2018-07-16T19:47:53Z

include/lsst/jointcal/ConstrainedPhotometryModel.h

    typedef std::map<VisitIdType, std::shared_ptr<PhotometryMapping>> VisitMapType;
    VisitMapType _visitMap;
+    // The per-chip transforms that go into _chipVisitMap.


So is _chipVisitMap an optimization to avoid recomputing the composition? It might be good to explicitly spell out the self-consistency requirements (which I assume are enforced by ConstrainedPhotometryModel).

refactor test_photometryMapping.py so that I can compute the chip and visit derivatives separately. The cfht ConstrainedPhotometryModel chi2 changed very slightly, which I can believe, as it starts with a different initial step.

parejkoj force-pushed the tickets/DM-15011 branch from 4d1df20 to f4b92e5 Compare July 6, 2018 01:37

kfindeisen requested changes Jul 7, 2018

View reviewed changes

kfindeisen reviewed Jul 16, 2018

View reviewed changes

kfindeisen approved these changes Jul 18, 2018

View reviewed changes

parejkoj force-pushed the tickets/DM-15011 branch from 7b768cd to 85f99d5 Compare July 19, 2018 00:29

parejkoj added 2 commits July 18, 2018 17:29

Add testUtils option to fake the ccdId

6188d0e

parejkoj force-pushed the tickets/DM-15011 branch from 85f99d5 to 10d889c Compare July 19, 2018 00:29

parejkoj merged commit 82b7855 into master Jul 19, 2018

ktlim deleted the tickets/DM-15011 branch August 25, 2018 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-15011 Add ModelVisit/ModelChip options for photometry #96

DM-15011 Add ModelVisit/ModelChip options for photometry #96

parejkoj commented Jul 6, 2018

kfindeisen left a comment

kfindeisen Jul 6, 2018

parejkoj Jul 13, 2018

kfindeisen Jul 6, 2018

parejkoj Jul 13, 2018

kfindeisen Jul 6, 2018

kfindeisen Jul 6, 2018

parejkoj Jul 13, 2018

kfindeisen Jul 6, 2018

parejkoj Jul 13, 2018

kfindeisen Jul 6, 2018

parejkoj Jul 13, 2018

kfindeisen Jul 16, 2018

kfindeisen Jul 6, 2018

parejkoj Jul 13, 2018

kfindeisen Jul 16, 2018

parejkoj Jul 18, 2018

kfindeisen Jul 6, 2018

kfindeisen Jul 6, 2018

parejkoj Jul 13, 2018

kfindeisen Jul 16, 2018

kfindeisen Jul 6, 2018

parejkoj Jul 13, 2018

kfindeisen Jul 16, 2018

kfindeisen Jul 16, 2018

		@@ -198,7 +198,7 @@ class ChipVisitPhotometryMapping : public PhotometryMappingBase {
		_visitMapping(std::move(visitMapping)) {}

DM-15011 Add ModelVisit/ModelChip options for photometry #96

DM-15011 Add ModelVisit/ModelChip options for photometry #96

Conversation

parejkoj commented Jul 6, 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

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