DM-11095 add refcat support for jointcal photometry #44

parejkoj · 2017-07-27T08:38:26Z

This work developed out of the start of DM-9195, and I had to complete it before doing 9195, but it's worth its own review first.

Add Constrained to PhotometryModel pybind11, and cleanup file

NOTE: doesn't work yet because PhotometryFit doesn't accept nOutliers. It probably should? But this is telling me something important about the difference between the as-implemented photometry and astrometry fitters. fix indentation in jointcal.py

Make astrometryFit.minimize() return int like photometry photometry code now compiles and python will run; doesn't fit (needs ref cat)

Start switching PhotometryFit to use Astrometry's Cholmod code

Load per-filter fluxes from refcat Add refCat flux loading to python Cleanup RefStar cruft

Move Chi2Entry/Chi2List out of *Fit and cleanup chi2 naming. Add computeAverageAndSigma() to Chi2List for *Fit to use.

Rename variables to better match document

…ames

Remove unnecessary line breaks Name and text cleanup

Correctly load refStar fluxErrs Improve chi2 and photometryTransfo output

Lift as many methods up into FitterBase as possible and refactor. Remove templating of chi2 accumulator and do it via inheritance. Lift getMapping into base PhotometryModel class. Add lots of missing documentation. Fix photomFactor use in jointcal.py

TallJimbo

I've finished a zoomed-in, detail-oriented review of all of the code, with lots of small style requests. I'd like to take more of a big-picture look next week to better understand how all of the classes fit together, and would love to hear any thoughts you have on how to approach that.

TallJimbo · 2017-07-27T21:04:18Z

doc/jointcal.tex

+algebra, the Cholmod and Eigen packages provide the required
+functionality. It turns out that for practical problems, the
+calculation of $J^TJ$ or the factorization are the most CPU intensive
+parts of the calculations, and


Interesting to note that in Bernstein (2017), the calculation of the matrix was more CPU intensive than the matrix factorization. The way we're fitting isn't exactly the same, but maybe this is a sign that with more complex models this statement may not hold.

I think it depends a lot on the models, but we'll have to see (e.g., our derivative approach saves us some work, I think). We'll definitely have to spend some time profiling jointcal in the future: I've reshuffled a lot of things, and haven't always been totally performance oriented (i.e., just make it work) in my thinking.

TallJimbo · 2017-07-27T21:12:26Z

include/lsst/jointcal/Associations.h

-                         afw::geom::Angle matchCut, std::string const &fluxField);
+                         afw::geom::Angle matchCut, std::string const &fluxField,
+                         std::map<std::string, std::vector<double> > const &refFluxMap,
+                         std::map<std::string, std::vector<double> > const &refFluxErrMap);


C++11 means not having to add a space between those angle brackets at the end of a nested template type.

I find it helps readability for me, otherwise I have trouble separating the brackets. We don't seem to have a documented standard on this.

TallJimbo · 2017-07-27T21:13:50Z

include/lsst/jointcal/Associations.h

@@ -43,7 +43,7 @@ class Associations {
     * Source selection is performed in python, so Associations' constructor
     * only initializes a couple of variables.
     */
-    Associations();
+    Associations() : _commonTangentPoint(Point(0, 0)) {}


I'm guessing this default is never supposed to be used, and if that's the case, NaN might be a better choice to make it more apparent if it accidentally is.

Thinking about it, you're probably right.

TallJimbo · 2017-07-27T21:26:29Z

include/lsst/jointcal/AstrometryFit.h

    bool _fittingDistortions, _fittingPos, _fittingRefrac, _fittingPM;
-    AstrometryModel *_astrometryModel;
-    int _LastNTrip;                   // last triplet count, used to speed up allocation
+    AstrometryModel &_astrometryModel;


I recommend against using references (const or otherwise) as data members. If you don't want any kind of ownership relationship between the two classes, I think raw pointer is a bit better because it makes it a bit more explicit what's going on (I'm guessing you have a reason why you can't use shared_ptr or weak_ptr instead here). I also think you need to delete or redefine all of the compiler-generated copy/move constructors and assignment operators for either to be safe.

I don't have any reason not to use a shared_ptr here: is there one? I think that'd be preferable to a raw pointer, but I'm not well versed enough to know better.

I've deleted the move/copy constructors/assignment for most things, with a comment as to why.

It's hard to know from just looking locally at the code whether shared_ptr will work; it's mostly a question of whether there would be cycles (unlike Python, shared_ptr doesn't do garbage collection).

After some offline discussion, we decided that shared_ptr was appropriate for this case: the Associations instance outlives any given fitter, and the Models must live at least as long as the fitter that works with them.

TallJimbo · 2017-07-27T21:33:50Z

include/lsst/jointcal/AstrometryFit.h

-
-    //! Produces both ntuples (cook up names from the provided string)
-    void makeResTuple(const std::string &tupleName) const;
+    void saveResultTuples(const std::string &tupleName) const;


I'm sure this is preexisting, so don't feel obligated to fix it on this ticket, but DM convention would be std::string const & tupleName.

Semi-unrelated doc request, just because I'm confused: what does this function do? Where does it save the things it claims to save?

I've gone and cleaned up a bunch of const T & things, and added a better docstring for that method. It's mostly for debugging, by dumping the all the current internal values to text files. We probably want something better eventually.

TallJimbo · 2017-07-28T20:30:06Z

src/FitterBase.cc

+         causing the large chi2 we have in hand.  */
+        bool drop_it = true;
+        for (auto const &i : indices)
+            if (affectedParams(i) != 0) drop_it = false;


Use braces after for and if.

TallJimbo · 2017-07-28T20:30:17Z

src/FitterBase.cc

+            // mark the parameters as directly changed when we discard this chi2 term.
+            for (auto const &i : indices) affectedParams(i)++;
+            nOutliers++;
+        }


Fix/use more braces.

TallJimbo · 2017-07-28T20:31:28Z

src/FitterBase.cc

+        int nOutliers = findOutliers(nSigmaCut, msOutliers, fsOutliers);
+        totalOutliers += nOutliers;
+        if (nOutliers == 0) break;
+        TripletList tripletList(1000);  // initial allocation size.


Bothered by the magic number. Should at least be a product of the number of stars and/or CCD images, right?

I'm pretty sure it is exactly the number of outliers computed above. Fixed, thanks.

TallJimbo · 2017-07-28T20:32:36Z

src/RefStar.cc

+// void RefStar::assignRefFluxes(std::vector<double> const &refFlux) {
+//     _refFlux.clear();
+//     copy(refFlux.begin(), refFlux.end(), back_inserter(_refFlux));
+// }


TallJimbo · 2017-07-28T20:58:19Z

tests/test_jointcal_cfht_minimal.py

+            raise unittest.SkipTest("testdata_jointcal not setup")
+
+    def setUp(self):
+        do_plot = False


Lots of nonconforming variable names in this file.

As above, I thought we were allowed to use pythonic _ names?

Everywhere except for Science Pipelines code.

So, wait, what does that leave? Where's the standard on this again?

https://developer.lsst.io/coding/python_style_guide.html#naming-conventions

I'll hold off on converting for this ticket.

parejkoj · 2017-07-29T10:04:11Z

I think I've responded/fixed to all of your comments with the latest commits. I'd love some suggestions regarding the data references as members questions (I'd be happy to chat on slack, so I can better understand why): I don't know if raw, shared, or other pointers are a better replacement. See also a few other questions above.

Add Chi2.cc pulled from Chi2.h in review Fix many "const T &" -> "T const &" Make minimize() return an enum type Explicit delete or default copy/move for many classes Have Fitter constructors set _nPar* variables to 0, instead of calling overridden assignIndices(), which makes further sub-classes hard to create. cleanup doxygen

parejkoj and others added 29 commits June 21, 2017 21:07

Add ConstrainedPhotometryModel header

0861576

Add Constrained to PhotometryModel pybind11, and cleanup file

Add constrainedPhotometry test

5e14e06

Add PhotometryTransfo w/pybind11

ce51a34

Convert SimplePhotometryModel to use PhotometryMapping

2690542

Make astrometryFit.minimize() return int like photometry photometry code now compiles and python will run; doesn't fit (needs ref cat)

Remove dead code from astrometryFit.cc

7ad3b04

Move CholmodSimplicialLDLT2 out of AstrometryFit and cleanup #includes

5cff3c9

Start switching PhotometryFit to use Astrometry's Cholmod code

Switch to PhotoCalib and make "flux/instFlux" usage consistent

fa9f536

Load per-filter fluxes from refcat Add refCat flux loading to python Cleanup RefStar cruft

rewrite PhotometryTransfo docstrings

4a2c87d

Cleanup Chi2-accumulators

1123af7

Move Chi2Entry/Chi2List out of *Fit and cleanup chi2 naming. Add computeAverageAndSigma() to Chi2List for *Fit to use.

initial PhotometryFit refstar LS code

9974a28

Cleanup photometry transfos fitter and model

59387b7

Some comments added after discussion with Pierre

e710bbd

Rename variables to better match document

Added more details about derivatives, and fixed a few class/routine n…

96a7e3f

…ames

Cleanup LaTeX doc

db2875e

Remove unnecessary line breaks Name and text cleanup

Rename tList->tripletList, fsl->fittedStarList

d913c69

some additions about derivatives

1a6b1c6

Add name property to ccdImage

1755baf

More work on photometry reference derivatives

cc2df68

Correctly load refStar fluxErrs Improve chi2 and photometryTransfo output

Log initialization chi2

09663a2

Add catalog id to measuredStar

4450bf0

Add 2-star cfht_minimal test for photometry

9b8609a

Add fluxErr to baseStar output

04668f9

Remove irrelevant index from RefStar

14a4433

Final updated test metrics to match new refcat photometry

dba4295

Add colors and legend to pa1 plots

5e60641

Make collectRefStars not blow up if refcat doesn't have fluxErrs

813dea4

Make test_wcs match new BaseStar constructor

36b92ac

TallJimbo reviewed Jul 28, 2017

View reviewed changes

parejkoj force-pushed the tickets/DM-11095 branch 2 times, most recently from 0ac89dc to 991f8f7 Compare July 29, 2017 10:04

parejkoj added 2 commits July 31, 2017 17:23

Cleanup TeX doc some more

69ad124

parejkoj force-pushed the tickets/DM-11095 branch from 3e6a04c to 447a761 Compare August 1, 2017 01:20

parejkoj merged commit 447a761 into master Aug 1, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-11095 add refcat support for jointcal photometry #44

DM-11095 add refcat support for jointcal photometry #44

parejkoj commented Jul 27, 2017

TallJimbo left a comment

TallJimbo Jul 27, 2017

parejkoj Jul 28, 2017

TallJimbo Jul 27, 2017

parejkoj Jul 28, 2017

TallJimbo Jul 27, 2017

parejkoj Jul 28, 2017

TallJimbo Jul 27, 2017

parejkoj Jul 28, 2017

parejkoj Jul 29, 2017

TallJimbo Jul 31, 2017

parejkoj Jul 31, 2017

TallJimbo Jul 27, 2017

parejkoj Jul 28, 2017

TallJimbo Jul 28, 2017

TallJimbo Jul 28, 2017

TallJimbo Jul 28, 2017

parejkoj Jul 29, 2017

TallJimbo Jul 28, 2017

TallJimbo Jul 28, 2017

parejkoj Jul 29, 2017

timj Jul 29, 2017

parejkoj Jul 29, 2017

timj Jul 29, 2017

parejkoj Jul 31, 2017

parejkoj commented Jul 29, 2017

DM-11095 add refcat support for jointcal photometry #44

DM-11095 add refcat support for jointcal photometry #44

Conversation

parejkoj commented Jul 27, 2017

TallJimbo 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

parejkoj commented Jul 29, 2017