DM-10728: write photometric fit results as a PhotoCalib with a custom BoundedField #15

TallJimbo · 2017-07-03T21:02:16Z

This hides all of the messy rotation logic inside a new C++ class, FluxFitBoundedField, and puts the results in a format that jointcal could also use. When combined with the new PhotoCalib class, this should provide everything we need to use the photometric fit results. This ticket currently tests the behavior of this class against the existing routines in updateExposure.py; a future ticket could now remove those routines in favor of simpler operations on PhotoCalib (though we may want to add a bit more to PhotoCalib and/or BoundedField to make that easier, such as mixed operations between BoundedField and MaskedImage).

laurenam · 2017-07-17T18:22:04Z

For the "Fix interpolation bugs in getFCorImg and getJImg. " commit, while I appreciate that you'd rather not split out the formatting fixes, it is really difficult as is to disentangle the two, especially without knowing the exact nature of the bugs being fixed. If you really don't want to split this into two commits (I'm not going to force it), could you at least spell out to some degree in the commit message what the nature of the interpolation bugs were and the fixes made? When I look at the images in your unittest, I do see some very strange patterns in the difference images that are different for each nQuarter and fcr vs. jacobian components (these are very low level patterns, but I'm curious...)

Also, I note that many of the formatting changes involve adding spaces around the multiplication and division operators. While it is only in Python that these should never be surrounded by whitespace (https://developer.lsst.io/coding/python_style_guide.html#binary-operators-should-be-surrounded-by-a-single-space-except-for vs. https://developer.lsst.io/coding/cpp_style_guide.html#the-following-white-space-conventions-should-be-followed), most of the other instances in this file omit the spaces, so this formatting change is not only not required, but makes the file slightly less internally consistent. Again, since this repo is probably more heterogeneous and guideline-violating than most in terms of format and style, I won't force your hand one way or the other, but thought I'd at least point it out.

TallJimbo · 2017-07-18T15:13:56Z

I've just pushed a rebase that splits off the indentation changes from the commit that fixes the interpolation bugs. A (mostly positive) side effect is that I've run clang-format on the entirety of those two files in the whitespace-only commit, which adds a lot of new changes, but you should now be able to see just the interpolation fixes in the following commit.

I believe the differences between the images produced by the old meas_mosaic code and the new code are down to the following:

For nQuarter != 0, there's a pretty big loss of numerical precision when we rotate the SIP terms in the WCSs using the code in meas_astrom. I would expect that to produce a pattern that's somehow related to the power in the higher-order polynomials, where the loss of precision is most severe. This is why I use a different threshold for nQuarter == 0 in the unit tests - it's possible to get much better agreement in that case.
The conventions of PhotoCalib require FluxFitBoundedField to output the flux at magnitude zero, which is the reciprocal of what getFCorImg outputs. Furthermore, the fact the interpolation has to go in the BoundedField base class means that those quantities are what we (linearly) interpolate, while getFCorImg instead linearly interpolates additive corrections in magnitude space and then exponentiates them after interpolating.
Because the above already makes it impossible for FluxFitBoundedField to agree exactly with the old code (except at non-interpolated points), I've written the tests to interpolate in both x and y for FluxFitBoundedField, since that should yield much better performance with no loss in overall precision (the old code interpolated only in x not because x is special, but because interpolating in both is harder to implement - but I've done that and tested it pretty thoroughly in afw).

laurenam · 2017-07-27T19:32:38Z

python/lsst/meas/mosaic/mosaicTask.py

+                dataRef.put(exp, "fcr")
+            except Exception as e:
+                print "failed to write fcr: %s" % (e)
+


It looks like this this try...except block is duplicated. Also, can you also change "something ==> wcs" at line 637 above?

laurenam · 2017-07-27T19:42:55Z

python/lsst/meas/mosaic/updateExposure.py

-                                                 errField.getElementCount())
+            newErrField = errField.__class__(newName + sigmaName,
+                                             "Calibrated magnitude error from %s (%s)" %
+                                             (errField.getName(), errField.getDoc()), "mag")
            newErrKeys[newName] = mapper.addMapping(errKeys[name + sigmaName], newErrField)


While it is true that LSST no longer uses array fields for flux values, I'm pretty sure this was allowing for data processed with the old-time HSC stack to be updated (e.g. aperture corrections are stored in an array field). I'm not sure if we will ever have a use case for running this on such an archaic processing run, the rest of the repo still accommodates it, so adding only this adaptation without removing all the others may lead to confusion.

This code was already broken by the time I got to it: getElementCount was never intended to be a public method, and it isn't anymore (as of the pybind11 port, I believe). So fixing this code would require rewriting it to avoid that check. Given that we shouldn't need to run the new version of meas_mosaic against the outputs of older versions of the rest of the pipeline anymore, I don't think that's worthwhile.

laurenam · 2017-07-27T19:46:27Z

src/mosaicfit.cc

@@ -1,21 +1,21 @@
-#include <strings.h>


Uh oh...looks like including this in this file commit reverts what you did on the clang-cleanup commit!#$!

Glad you noticed these, and I'm embarrassed I didn't. Should be fixed now.

laurenam · 2017-07-27T19:49:33Z

src/fluxfit.cc

@@ -201,7 +201,7 @@ Eigen::VectorXd fluxFit_rel(std::vector<Obs::Ptr> &m, int nmatch, std::vector<Ob
        }
    }

-    int ncoeff_offset = 3;  // Fit from 2nd order only
+    int ncoeff_offset = 3;	// Fit from 2nd order only
    // In some cases (small number of visits or small dithering),


This is reverting the formatting done on the clang-cleanup commit: lsst-dm/ancient-legacy-meas_mosaic@0caf217#diff-55b138c02dda9dcf23718ad652bf219eR204

laurenam · 2017-07-27T19:49:39Z

src/fluxfit.cc

@@ -963,7 +963,7 @@ Eigen::VectorXd fluxFit_abs(std::vector<Obs::Ptr> &m, int nmatch, std::vector<Ob
        }
    }

-    int ncoeff_offset = 1;  // Fit from 1st order only
+    int ncoeff_offset = 1;	// Fit from 1st order only
    FluxFitParams::Ptr p = ffpSet[ffpSet.begin()->first];


Ditto: lsst-dm/ancient-legacy-meas_mosaic@0caf217#diff-55b138c02dda9dcf23718ad652bf219eR966

laurenam · 2017-07-27T19:54:48Z

python/lsst/meas/mosaic/fluxFitBoundedField.cc

+/*
+ * LSST Data Management System
+ * Copyright 2008-2016  AURA/LSST.
+ *


timj · 2017-07-27T21:37:57Z

python/lsst/meas/mosaic/mosaicTask.py

@@ -672,7 +672,40 @@ def writeFcr(self, dataRefList):
            try:
                dataRef.put(exp, "fcr")
            except Exception as e:
-                print "failed to write something: %s" % (e)
+                print "failed to write fcr: %s" % (e)


This code has to be ported to support python3.

Given that the point of this work is to enable us to retire meas_mosaic (by demonstrating that Jointcal is an adequate replacement), I'd rather not spend time on that.

clang-format doesn't do things as nicely as I would have done by hand, but it's infinitely better than the non-indented nests of for-loops and mix of tabs and spaces we had before.

Includes titles, scaling, and mask transparency for the images displayed on ds9 as well as an option to pause and drop into the pdb debugger to allow for inspection of the displayed images.

TallJimbo force-pushed the tickets/DM-10728 branch from 50c8e6f to 2cae5d0 Compare July 18, 2017 15:03

laurenam reviewed Jul 27, 2017

View reviewed changes

timj reviewed Jul 27, 2017

View reviewed changes

TallJimbo force-pushed the tickets/DM-10728 branch 2 times, most recently from 07ea342 to 24cbbab Compare July 28, 2017 15:45

TallJimbo added 4 commits July 28, 2017 12:05

Add class to view photometric results with cameraGeom.showCamera

9ef91a0

Fix indentation in src files with clang-format.

1cab64a

clang-format doesn't do things as nicely as I would have done by hand, but it's infinitely better than the non-indented nests of for-loops and mix of tabs and spaces we had before.

Fix interpolation bugs in getFCorImg and getJImg.

2f9e910

Add BoundedField wrapper for fluxfit results.

9f27397

TallJimbo force-pushed the tickets/DM-10728 branch from 24cbbab to 87d34c9 Compare July 28, 2017 16:05

TallJimbo added 3 commits July 28, 2017 12:17

Write photometric fit result to photoCalib dataset.

40577d4

Array fields are no longer used for flux values.

8e9e291

Add test for using FluxFitBoundedField in PhotoCalib.

9b53371

TallJimbo force-pushed the tickets/DM-10728 branch from 87d34c9 to 6ae3801 Compare July 28, 2017 16:17

laurenam added 2 commits July 28, 2017 14:54

testFluxFitBoundedField: update display parameters

4fd096a

Includes titles, scaling, and mask transparency for the images displayed on ds9 as well as an option to pause and drop into the pdb debugger to allow for inspection of the displayed images.

Add class to view photoCalib results with cameraGeom.showCamera

33302a9

TallJimbo force-pushed the tickets/DM-10728 branch from 6ae3801 to 33302a9 Compare July 28, 2017 18:55

TallJimbo merged commit b603521 into master Jul 28, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-10728: write photometric fit results as a PhotoCalib with a custom BoundedField #15

DM-10728: write photometric fit results as a PhotoCalib with a custom BoundedField #15

TallJimbo commented Jul 3, 2017

laurenam commented Jul 17, 2017

TallJimbo commented Jul 18, 2017 •

edited

laurenam Jul 27, 2017

laurenam Jul 27, 2017

TallJimbo Jul 28, 2017

laurenam Jul 27, 2017

TallJimbo Jul 28, 2017

laurenam Jul 27, 2017

laurenam Jul 27, 2017

laurenam Jul 27, 2017

timj Jul 27, 2017

jdswinbank Jul 28, 2017 •

edited

DM-10728: write photometric fit results as a PhotoCalib with a custom BoundedField #15

DM-10728: write photometric fit results as a PhotoCalib with a custom BoundedField #15

Conversation

TallJimbo commented Jul 3, 2017

laurenam commented Jul 17, 2017

TallJimbo commented Jul 18, 2017 • 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

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

jdswinbank Jul 28, 2017 • edited

Choose a reason for hiding this comment

TallJimbo commented Jul 18, 2017 •

edited

jdswinbank Jul 28, 2017 •

edited