The goal of this exercise is to run the newly minted algorithms on real (well, lsstSim) data, and see how the results match up with the existing meas_algorithm framework

This is the code fixes which came out of this review. I will put the results of the review on a wiki page once the PsfFlux bugs are fixed.

tickets/DM-445 in meas_base
[pgee@lsst-dev mb5]$ git diff --stat master
include/lsst/meas/base/SdssCentroid.h | 3 ++-
include/lsst/meas/base/SdssShape.h | 4 ++--
python/lsst/meas/base/plugins.py | 2 +-
src/Inputs.cc | 2 +-
src/SdssShape.cc | 16 +++++++---------
tests/testClassificationBasic.py | 2 +-
6 files changed, 14 insertions, 15 deletions

Add setVersion to the creation of the measurement table:
[pgee@lsst-dev pipe_tasks]$ git diff --stat master
python/lsst/pipe/tasks/processImage.py | 2 ++
1 files changed, 2 insertions, 0 deletions

Changes to Source.h in u/pgee/s14
[pgee@lsst-dev afw4]$ git diff --stat master
include/lsst/afw/table/Source.h.m4 | 28 ++++++++++++++++++++++++++--
1 files changed, 26 insertions, 2 deletions

I've just pushed two commits on meas_base tickets/DM-445:

• Fixed the differences between the new and old PsfFlux. See the commit message for more details.
• I added a new approach to setting the default executionOrder for C+-implemented plugins, so it's no longer necessary to include that in the C+ control objects.

I'll proceed with the rest of the review tomorrow.

I'm not sure why you would change the execution order methodology on this
ticket. Doesn't that belong in another ticket?

I pushed the change to tickets/DM-445. I did not realize it could not be a part of the review in u/pgee/s14. It is the same set of changes, in either case.

There were a couple of bugfixes included, which hopefully the log reflects. I encountered them during my testing.

The only change which does not have to be checked in for the code to work is the change to SourceRecord::getCentroid() to return the peak centroid if the result from the centroid slot is nan. I did this to make the Centroid result compatible with the old meas_algorithms, which allowed the centroid to be set even if the centroid algorithm ultimately failed.

I actually think that we should remove this behavior, and let the Centroid by Nan if the algorithm fails.

I'm not sure why you would change the execution order methodology on this ticket. Doesn't that belong in another ticket?

I saw that you'd added executionOrder fields to the control objects on this branch, in order to make the centroids happen in the right order. I was planning to bring that up in the review, as I wanted to avoid putting that on master if possible. But I didn't know what to suggest instead, so I experimented a bit, and it turned out the solution was so easy I just went ahead and committed it.

I pushed the change to tickets/DM-445. I did not realize it could not be a part of the review in u/pgee/s14. It is the same set of changes, in either case.

Thanks; it's not so much that it couldn't be part of the review on the other branch, it's more that I didn't know if it was intended to be or if it was all just supposed to be temporary

There were a couple of bugfixes included, which hopefully the log reflects. I encountered them during my testing.
The only change which does not have to be checked in for the code to work is the change to SourceRecord::getCentroid() to return the peak centroid if the result from the centroid slot is nan. I did this to make the Centroid result compatible with the old meas_algorithms, which allowed the centroid to be set even if the centroid algorithm ultimately failed.

I actually think that we should remove this behavior, and let the Centroid by Nan if the algorithm fails.

I agree on all counts. The bugfixes are clearly necessary, and hasCentroid() looks like a useful addition, but I'd like to remove the extra check in getCentroid() before merging to master.

This is a test to see to whom this comment gets attributed.

Review complete.

My only major concern is that I'd like to replace the mechanism for choosing table versions in pipe_tasks; I don't think we want to have a separate config option that has to be set by the user to match the version expected by the measurement task. Here's a proposal for an alternative: we add a TABLE_VERSION class attribute constant to the measurement tasks (both the new one and the old one), and have ProcessImageTask do table.setVersion(self.measurement.TABLE_VERSION). That'd require opening up a branch for this issue in meas_algorithms, but I think it's worthwhile.

I also have two minor issues:

• I don't think the code to chose the highest numerical flag in SdssShape has any value. Granted, we can't do the right thing on this ticket, but I think this extra logic adds confusion without any additional value. If you choose to leave it, add a comment that it's completely ad-hoc and needs to be replaced.
• The indentation in Inputs.cc is off, in FootprintCentroidInput's ctor.

Finally, looking at the concerns you brought up in your email that still need to be resolved, we should update some tickets:

1. Cannot both set flags and return values in the new framework.

I see you've already filed this as DM-688.

2. It seems obvious to me that we should adopt standards for EDGE, NOPIXELS, NOPSF and NOWCS errors so that all algorithms handle these things in the same way.

Good idea. I could see this being part of DM-461, or I could see it being a new issue; I'll leave which up to you. Could you either add this as a note to DM-461 or open a new issue for it?

3. More consistency in the configuration options between the fluxes.

I think you're probably right in general, but I'd like to know what you mean specifically. But please just open a new issue and describe it there.