Support multiple-aperture fluxes in slots

XMLWordPrintable

Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• Story Points:
2
• Sprint:
Science Pipelines DM-W15-3, Science Pipelines DM-W15-4
• Team:
Data Release Production

Description

We should be able to use multiple-aperture flux results in slots. While this is technically possible already by setting specific aliases, it doesn't work through the usual mechanisms for setting up slots (the define methods in SourceTable and the SourceSlotConfig in meas_base).

After addressing this, we should remove the old SincFlux and NaiveFlux algorithms, as the new CircularApertureFlux algorithm will be able to do everything they can do.

Activity

Hide
Jim Bosch added a comment -

Suspending work on this until RFC is resolved.

Show
Jim Bosch added a comment - Suspending work on this until RFC is resolved.
Hide
Jim Bosch added a comment -

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.

Show
Jim Bosch added a comment - 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.
Hide
Jim Bosch added a comment -

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  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  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  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  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  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  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  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  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  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  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  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  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  Date: Fri Jan 23 16:20:03 2015 -0500    Add .gitignore to exclude outputs from version control    .gitignore | 3 +++  1 file changed, 3 insertions(+)

Show
Jim Bosch added a comment - 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(+)
Hide
John Swinbank added a comment -

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.

Show
John Swinbank added a comment - 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.
Hide
Jim Bosch added a comment -

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.

Show
Jim Bosch added a comment - 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.
Hide
John Swinbank added a comment -

I've now looked at the changes to pex_config & have no objections. Feel free to go ahead and merge them.

Show
John Swinbank added a comment - I've now looked at the changes to pex_config & have no objections. Feel free to go ahead and merge them.

People

• Assignee:
Jim Bosch
Reporter:
Jim Bosch
Reviewers:
John Swinbank
Watchers:
Jim Bosch, John Swinbank