Hi Perry, just to be clear – you'd like Lauren to review that, if you could track down her contact details in JIRA. Is that correct?

I went ahead and changed the reviewer to Lauren MacArthur. Lauren, when you get this, come by my office, and I'll walk you through the code review procedure.

Before checking in this issue, we need to test to see what has changed in the measurement algorithms. Since buildBot depends on the sdss demo, we need in reality to do this every time we make a change to meas_base.

There were a few changes in the algorithms during this work which created differences in the LsstSim test of a single image with 4500 sources:

1. The differences in several of the algorithms vs the current master were created by the change where the SdssShape flag is checked by GaussianFlux, and the measurement does not proceed if the flag is set. So all of these values end up with NaN values for the Gaussian flux, and that also percolates into differences in classification_extendedness, which uses the model flux slot.

2. The PixelFlags also differ for edges since we removed the edge check.

This all looks fine to me. I list some comments below, but they are
largely just minor suggestions for consistency.

Comments by file name:

GaussianFlux.h:

• remove commented //#include "lsst/meas/base/ShapeUtilities.h"
  (or state why it's still there)?
• does enum need to be named? Probably does not matter, but to be
  consistent with others, e.g. PsfFlux.h, SdssShape.h (NOTE: this
  also occurs in the other .h files here).

Editorial (changes marked btw asterisks):

• line 60 "The size and ellipticity of the weight function are determined..."
• line 79 "These are private so they don't shadow..."
• line 80 "we can still call them via..."

NOTE: the latter two above are in all .h files here.

GaussianFlux.cc:

• do you also need #include "boost/array.hpp" (as in PsfFlux.cc)?
• line 66: get rid of - mimage.getX0(); in comment?
• line 67: get rid of - mimage.getY0(); in comment?

• Could get rid of lines 64-67 if line 72 read:
  = detail::getFixedMomentsFlux(exposure.getMaskedImage(), _ctrl.background, centroid.getX(), centroid.getY(), sdss);

• There are 4 instances of whitespace

NaiveFluc.h:

• all ok

NaiveFluc.cc:

• line 165: name consistency: what was called "centroid" in GaussianFlux.cc
  is here called "center"
• There are many instances of whitespace

PeakLikelihoodFlux.h:

• all ok

PeakLikelihoodFlux.cc:

• line 196: name consistency: what was called "centroid" in GaussianFlux.cc
  is here called "center"
• There are a few instances of whitespace

SincFlux.h:

• all ok

SincFlux.cc:

• lines 39-41: collapse to single line as in other .cc files here,
  i.e. to:
  namespace lsst { namespace meas { namespace base {
  ,
• line 193: name consistency: what was called "centroid" in GaussianFlux.cc
  is here called "center"
• There are a few instances of whitespace

testGaussianFluxBasic.py:

• I'm not sure why self.assertTrue(record.get("base_GaussianFlux_flag"))
  was commented out on line 75. If it's not needed, why not just remove
  the line?

plugins.py:

• all ok

pluginsLib.i:

• could still comment with
  // Flux algorithms
  ...
  // Centroid algorithms
  ...
  // Miscellaneous algorithms

PixelFlags.cc:

• There are a few instances of whitespace

Converted this from Won't Fix to Done, as I think setting it to the former was a mistake: note that when you drag the issue around one the agile "Work" page, there are now two boxes in the rightmost column, so it's easy to accidentally drag it to Won't Fix if you're not careful.