DM-10728: miscellaneous changes in support of PhotoCalib outputs in meas_mosaic #253

TallJimbo · 2017-07-03T20:57:56Z

No description provided.

parejkoj · 2017-07-04T00:46:51Z

My first concern is your change of _instFlux[Err] -> _flux[Sigma]; I tried very hard to get PhotoCalib self-consistent in its use of instFlux and Err.

Talking with @SimonKrughoff about this just now, the question is whether you can hold off a month or two for us to make the big instFlux and Sigma->Err changes, or whether we make this change (making PhotoCalib no longer self-consistent), and revert it later? Thoughts?

parejkoj

Handful of comments, but the big question was my previous comment re: instFlux.

parejkoj · 2017-07-04T01:00:46Z

python/lsst/afw/math/chebyshevBoundedField.cc

-    clsChebyshevBoundedField.def("__eq__", &BoundedField::operator==, py::is_operator());
-    clsChebyshevBoundedField.def("__ne__", &BoundedField::operator!=, py::is_operator());
+    /* Operators are defined only in the BoundedField base class;
+       we let Python inheritance provide them here. */


Nice improvement.

parejkoj · 2017-07-04T01:02:04Z

src/image/Calib.cc

@@ -285,7 +284,7 @@ std::pair<ndarray::Array<double, 1>, ndarray::Array<double, 1>> Calib::getMagnit
            continue;
        }
        double f, df;
-        convertToMagWithErr(&f, &df, _fluxMag0, fluxMag0InvSNR, *fluxIter, *fluxErrIter);


Wow, it was even more internally-inconsistent than I'd previously thought?

parejkoj · 2017-07-04T01:12:17Z

src/math/BoundedField.cc

+            y0 = y1;
+            y1 += yStep;
+        }
+        {


What's this second bracket-block for?

parejkoj · 2017-07-04T01:20:32Z

src/math/BoundedField.cc

+                z00 = z10;
+                z01 = z11;
+            }
+            {


What's this second bracket-block for?

It's conceptually a special-cased last iteration of the loop, and I like to do that at the same/scope indentation level as the rest of the loop body. I'll add a comment to that effect.

parejkoj · 2017-07-04T01:23:43Z

src/math/BoundedField.cc

+        // Local function to do bilinear interpolation using points
+        // [(x0, y0), (x0, y1), (x1, y0), (x1, y1)] with values
+        // [z00, z01, z10, z11], in pixels [x0:xEnd, y0:yEnd]
+        auto interpolate = [&img, &functor](


I feel like interpolate and processRowBlock are complicated enough that they should be anonymous functions instead of lambdas. I'd strongly make that case in python (or that they at least be inner functions), but I don't know what the standard is in C++, and our dev guide doesn't even mention lambdas.

I'm still learning when use C++ lambdas myself, but I like the way this encapsulates the local functions inside the only scope where they're used. Would be interested in @pschella's and @kfindeisen's opinions on best-practices here.

Hmm... dejà vu.

I agree with @parejkoj that these lambdas are way too long and that they should be proper functions (C++ doesn't have anonymous functions, BTW). Having this code in the middle of applyToImage just makes it hard to read (I already mistook the end of interpolate for the end of applyToImage, for example). Lambdas are designed for the strategy pattern and, to a lesser degree, functional programming, and neither seems to be happening here.

Also, this is a very long parameter list with identical/compatible types. Is there some way to factor it (XYZ triplets, BoundingBoxes, etc.)?

I've refactored the interpolation code along both of these lines (no more lambdas, factored parameter lists). Please feel welcome (but not compelled) to take another look.

parejkoj · 2017-07-04T01:24:13Z

tests/test_chebyshevBoundedField.py

+        field.fillImage(image2, xStep=3)
+        field.fillImage(image3, yStep=4)
+        field.fillImage(image4, xStep=3, yStep=4)
+        self.assertFloatsAlmostEqual(image1.array, image2.array, rtol=2E-2, atol=2E-2)


Shouldn't the tolerances for these each be different, since the step sizes are different?

TallJimbo · 2017-07-04T21:53:32Z

The change from instFlux->flux (etc) is a single commit that can and should be reverted easily later. Until the SourceCatalogs we produce adopt the new conventions, I think it's much more valuable to have usable code than self-consistent naming.

And as much as I'm in favor of switching to the new conventions, I'm worried that backwards compatibility requirements may drag that conversion out longer than we'd all like.

This can drastically reduce the evaluation time for large images, as it avoids a virtual function call at every pixel and allows a lot more compiler optimization. An interface for vectorized evaluation of BoundedField would also be good, but it's a much harder design problem.

Inheriting from PersistableFacade in subclasses isn't necessary, because pybind11 automatically downcasts C++ pointers when converting to Python, and downcasting is all PersistableFacade does. Removing that inheritance lets derived BoundedFields inherit operators (especially equality operators), which would otherwise go to the implementation PersistableFacade inherits from object. See DM-10899 for more information.

This may need to be reverted as part of the implementations of RFCs 333 and 356, but until then we should prefer working code to future-proof code.

Old code overestimated the contribution from fluxMag0Err by dividing it by the source flux instead of fluxMag0.

parejkoj reviewed Jul 4, 2017

View reviewed changes

TallJimbo force-pushed the tickets/DM-10728 branch from 983c07d to 7c49b63 Compare July 14, 2017 20:56

TallJimbo force-pushed the tickets/DM-10728 branch 2 times, most recently from c718b8f to 8169830 Compare July 28, 2017 15:47

TallJimbo added 7 commits July 28, 2017 11:47

Fix string formatting in slicing error message.

d01a66d

Wrap BoundedField PersistableFacade interface.

2623f6b

Use current conventions for flux field names.

662c67d

This may need to be reverted as part of the implementations of RFCs 333 and 356, but until then we should prefer working code to future-proof code.

Fix propagation of uncertainty in Calib

b782dbc

Old code overestimated the contribution from fluxMag0Err by dividing it by the source flux instead of fluxMag0.

Tighten tolerances in BoundedField interpolation test.

5a2997e

TallJimbo force-pushed the tickets/DM-10728 branch from 8169830 to 5a2997e Compare July 28, 2017 15:47

TallJimbo merged commit 036366c into master Jul 28, 2017

ktlim deleted the tickets/DM-10728 branch August 25, 2018 06:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-10728: miscellaneous changes in support of PhotoCalib outputs in meas_mosaic #253

DM-10728: miscellaneous changes in support of PhotoCalib outputs in meas_mosaic #253

TallJimbo commented Jul 3, 2017

parejkoj commented Jul 4, 2017

parejkoj left a comment

parejkoj Jul 4, 2017

parejkoj Jul 4, 2017

parejkoj Jul 4, 2017

parejkoj Jul 4, 2017

TallJimbo Jul 4, 2017

parejkoj Jul 4, 2017

TallJimbo Jul 4, 2017

kfindeisen Jul 5, 2017

kfindeisen Jul 5, 2017 •

edited

TallJimbo Jul 25, 2017

parejkoj Jul 4, 2017

TallJimbo commented Jul 4, 2017

DM-10728: miscellaneous changes in support of PhotoCalib outputs in meas_mosaic #253

DM-10728: miscellaneous changes in support of PhotoCalib outputs in meas_mosaic #253

Conversation

TallJimbo commented Jul 3, 2017

parejkoj commented Jul 4, 2017

parejkoj 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

kfindeisen Jul 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TallJimbo commented Jul 4, 2017

kfindeisen Jul 5, 2017 •

edited