Jim Bosch added a comment - 08/Jan/15 1:21 PM Suspending work on this until RFC is resolved.

Jim Bosch added a comment - 20/Jan/15 11:13 AM The resolution from RFC-9 is that we'll be changing the measurement algorithms to match the name suffixes expected by the slots, with algorithms that cannot do this directly creating their own aliases in order to conform. Thus, most of the work on this issue will actually involve changes to the aperture flux algorithms.

Jim Bosch added a comment - 23/Jan/15 2:43 PM John, I've got a moderately-sized review for you here. The discussion of the design is on RFC-9 , and the major changes for that are in meas_base, but there are a few other changes in other packages (related minor bugs, changes to downstream code to adapt to meas_base changes), all on tickets/ DM-1218 branches (aside from the demo package, for which I had to use u/jbosch/ DM-1218 ). afw: $ git --no-pager log --stat --reverse LSST/master..tickets/DM-1218 commit c2694464c92c8e628a6706d23bc2d206d8dcdcc1 Author: Jim Bosch <jbosch@astro.princeton.edu> Date: Tue Dec 30 11:35:41 2014 -0500   Test, fix replacing an existing alias in AliasMap   src/table/AliasMap.cc | 2 +- tests/testTableAliases.py | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-)   commit 01f4bf7c675fb0ea1a3fd1271dbfe6f6e4f7a3d9 Author: Jim Bosch <jbosch@astro.princeton.edu> Date: Wed Jan 21 18:22:06 2015 -0500   Correctly handle slots being undefined. Prior to this change, slots were only allowed to be undefined when the SourceTable is constructed, and removing slot aliases caused an exception to be thrown. Now, removing a slot alias simply resets that slot.   src/table/slots.cc | 142 +++++++++++++++++++++++++++++++++++++++++++++-------------------------------- tests/testSourceTable.py | 13 +++++++ 2 files changed, 96 insertions(+), 59 deletions(-) meas_base: $ git --no-pager log --stat --reverse LSST/master..tickets/DM-1218 commit 2d4da401d659d98af218ba018d736df7c942dbb0 Author: Jim Bosch <jbosch@astro.princeton.edu> Date: Wed Jan 14 12:48:54 2015 -0500   Give BasePlugin a non-trivial constructor. This guarantees that BasePlugin instances have the attributes set there without having to assume anything about their subclasses.   python/lsst/meas/base/base.py | 11 +++++++++++ python/lsst/meas/base/forcedMeasurement.py | 4 +--- python/lsst/meas/base/sfm.py | 4 +--- 3 files changed, 13 insertions(+), 6 deletions(-)   commit 567ec8cb95926eafd8c04fea21288a31944448b9 Author: Jim Bosch <jbosch@astro.princeton.edu> Date: Wed Jan 14 13:25:40 2015 -0500   Move plugin initialization to BaseMeasurementTask This removes some code duplication at the expense of a bit of opaque argument-forwarding. It'll become more important shortly, as the now-shared code is about to grow.   python/lsst/meas/base/base.py | 22 ++++++++++++++++++++++ python/lsst/meas/base/forcedMeasurement.py | 10 ++-------- python/lsst/meas/base/sfm.py | 12 +----------- 3 files changed, 25 insertions(+), 19 deletions(-)   commit f9865255059b561c28abc833893a67f0bee119f5 Author: Jim Bosch <jbosch@astro.princeton.edu> Date: Wed Jan 21 13:43:12 2015 -0500   Reorganize ApertureFlux outputs for consistency with slots. This changes aperture flux outputs to look more like an array-of-structs rather than a struct of array, e.g. "base_CircularApertureFlux_[0,1,2]_flux" instead of "base_CircularApertureFlux_flux_[0,1,2]". This makes it its field suffixes compatible with the slot mechanism.   include/lsst/meas/base/ApertureFlux.h | 78 ++++++++++++++++++++++----------------------- src/ApertureFlux.cc | 110 +++++++++++++++++++++++++++++----------------------------------- src/CircularApertureFlux.cc | 6 ++-- tests/measureSources.py | 2 +- tests/testApertureFlux.py | 19 +++++------ 5 files changed, 102 insertions(+), 113 deletions(-)   commit 514d9f61bd16a8ffa706abfc0fe993b07f28944d Author: Jim Bosch <jbosch@astro.princeton.edu> Date: Thu Jan 22 13:54:12 2015 -0500   Remove NaiveFlux and SincFlux, replacing them with CircularApertureFlux Now that it can participate in slots, CircularApertureFlux can do everything NaiveFlux and SincFlux once did, so there's no need for all that code duplication and boilerplate.   examples/runSingleFrameTask.py | 5 +- include/lsst/meas/base.h | 2 - include/lsst/meas/base/NaiveFlux.h | 114 ------------------------------- include/lsst/meas/base/SincFlux.h | 114 ------------------------------- python/lsst/meas/base/base.py | 2 +- python/lsst/meas/base/forcedMeasurement.py | 3 +- python/lsst/meas/base/plugins.py | 2 - python/lsst/meas/base/pluginsLib.i | 6 -- python/lsst/meas/base/sfm.py | 3 +- src/NaiveFlux.cc | 206 -------------------------------------------------------- src/SincFlux.cc | 215 ----------------------------------------------------------- tests/measureSources.py | 19 ------ tests/testClassification.py | 50 +++++++------- tests/testForced.py | 5 +- tests/testNaiveFlux.py | 97 --------------------------- tests/testSincFlux.py | 99 --------------------------- 16 files changed, 33 insertions(+), 909 deletions(-)   commit 13e2f8c4a3aaaca9c03ef06baa66448f0a2d531e Author: Jim Bosch <jbosch@astro.princeton.edu> Date: Thu Jan 22 14:01:01 2015 -0500   Move ApertureFlux methods to protected to fix Python signature errors A "using" declaration to un-shadow the multiple overloads of measure() doesn't appear to work with Swig, so we move those methods to protected to make Swig continue to call throught the base class interface, which works fine.   include/lsst/meas/base/ApertureFlux.h | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) meas_algorithms: $ git --no-pager log --stat --reverse LSST/master..tickets/DM-1218 commit e970673179cb0625f6efe5b78be04d6b96ad9f16 Author: Jim Bosch <jbosch@astro.princeton.edu> Date: Thu Jan 22 18:15:49 2015 -0500   Update tests to adapt to removal of NaiveFlux and SincFlux These have now been replaced by CircularApertureFlux.   python/lsst/meas/algorithms/secondMomentStarSelector.py | 6 +++--- tests/measure.py | 20 ++++++++++---------- tests/psfIO.py | 6 +++--- tests/testPsfDetermination.py | 6 +++--- 4 files changed, 19 insertions(+), 19 deletions(-) pipe_tasks: $ git --no-pager log --stat --reverse LSST/master..tickets/DM-1218 commit 63e038bcbc75b43299e2f880d57e5c2f6cff9425 Author: Jim Bosch <jbosch@astro.princeton.edu> Date: Fri Jan 23 16:40:13 2015 -0500   Adapt to removal of SincFlux in favor of CircularApertureFlux.   examples/measurePsfTask.py | 5 +++-- python/lsst/pipe/tasks/measurePsf.py | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) obs_sdss: $ git --no-pager log --stat --reverse LSST/master..tickets/DM-1218 commit 630bcbe48271e3fd41725b46e824372c618d5c3f Author: Jim Bosch <jbosch@astro.princeton.edu> Date: Fri Jan 23 13:12:44 2015 -0500   Adapt to removal of SincFlux in favor of CircularApertureFlux   python/lsst/obs/sdss/calibrate.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) lsst_dm_stack_demo (u/jbosch/ DM-1218 ): $ git --no-pager log --stat --reverse origin/master..u/jbosch/DM-1218 commit b57ddae9f48667080c8a8fd46bdcd04b0d5bb2e4 Author: Jim Bosch <jbosch@astro.princeton.edu> Date: Fri Jan 23 16:18:21 2015 -0500   Update to reflect removal of SincFlux in favor of CircularApertureFlux The changes in the 'expected results' files are due to a change from a default radius of 7 to 6 (pixels) when we switched algorithms. Before updating the files, I also verified that if we run with a 7 pixel aperture with the new code, the results match the old algorithm (aside from round-off error difference due to a slightly different implementation).   bin/export-results | 4 +- detected-sources.txt.expected | 9400 ++++++++++++++++++++++++++++++++--------------------------------- detected-sources_small.txt.expected | 6348 ++++++++++++++++++++++---------------------- 3 files changed, 7876 insertions(+), 7876 deletions(-)   commit e2045f303c571650e7c8f21c59fd4eba093392dd Author: Jim Bosch <jbosch@astro.princeton.edu> Date: Fri Jan 23 16:18:40 2015 -0500   Remove deleted algorithms entirely from export-results.   bin/export-results | 2 -- 1 file changed, 2 deletions(-)   commit 7b025e4ced6d0739d2a28596105e300c027b3a5f Author: Jim Bosch <jbosch@astro.princeton.edu> Date: Fri Jan 23 16:20:03 2015 -0500   Add .gitignore to exclude outputs from version control   .gitignore | 3 +++ 1 file changed, 3 insertions(+)

John Swinbank added a comment - 26/Jan/15 6:38 PM Looks pretty good to me. I made some comments on GitHub pull requests for the changes to afw and meas_base , but I don't think there's anything fundamental. (Confusingly, some of my comments on the meas_base PR were on specific commits, and some were on the overall diff. I didn't realise that GitHub would format them differently, but it seems that it does. I think it's still clear what I commented on, but if anything is ambiguous please do ask.) I also re-ran the stack demo, and confirmed the results on OS X are consistent with your numbers. I note there's a tickets/ DM-1218 branch on pex_config , but which you don't mention in your comment above. I think that this branch is only relevant to the first proposal in RFC-9 , and is obsolete given the solution adopted. I've not therefore reviewed it beyond a cursory glance. It might be worth marking that branch as not relevant for this ticket to prevent confusion when we stumbled across it in 6 months and forget why it wasn't merged. Since it also fixes some "very minor problems in the test code for DictField", it might be worth merging just those fixes to master.

Jim Bosch added a comment - 27/Jan/15 2:07 PM I believe I've addressed all the concerns in the GitHub comments. Your analysis of the pex_config branch was correct; it was only necessary for the first proposal in RFC-9 , and I actually thought I'd never pushed it. Looks like I don't have permission to delete it now that I have, and I suppose that might be considered poor form. In any case, looking at it again for the first time in a few weeks, I think I'd go a step further than just merging the fixes, as I think the rest of it is also worth keeping, even if it doesn't have an immediate use case. Would you mind taking a quick look at that code, just to get complete coverage on the code review? In the meantime, I'm going to start merging the rest, as it doesn't depend on these changes at all.

John Swinbank added a comment - 27/Jan/15 6:39 PM I've now looked at the changes to pex_config & have no objections. Feel free to go ahead and merge them.