Lauren MacArthur added a comment - 05/Mar/15 11:41 AM This fix has been pulled in on branches u/lauren/ DM-1738 in afw and meas_deblender. A minor fix was necessary to remove the trailing Exception to the variable name RuntimeErrorException in two files: src/detection/Footprint.cc & tests/footprint1.py. The fix is working as advertised, as seen in the attached M31_before & M31_after images. Here is the git summary: afw[u/lauren/DM-1738] $ git --no-pager log --stat --reverse origin/master.. commit 94548659e820c65ab8c7f9a1373f4450a18df3ba Author: Jim Bosch <jbosch@astro.princeton.edu> Date: Wed Apr 16 17:20:02 2014 -0400   Add Footprint method to merge with neighbors Conflicts: tests/footprint1.py   include/lsst/afw/detection/Footprint.h | 9 +++++++++ python/lsst/afw/detection/footprints.i | 8 ++++---- src/detection/Footprint.cc | 25 +++++++++++++++++++++++++ tests/footprint1.py | 27 +++++++++++++++++++++++++-- 4 files changed, 63 insertions(+), 6 deletions(-)   commit 0dd62783a47b1505c4492ceddd5f138de7cd77d2 Author: Lauren MacArthur <lauren@astro.princeton.edu> Date: Wed Mar 4 18:25:31 2015 -0500   Update Exception in Footprint and tests/footprint1.py to new name   src/detection/Footprint.cc | 2 +- tests/footprint1.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)   meas_deblender[u/lauren/DM-1738] $ git --no-pager log --stat --reverse origin/master.. commit 450d7cf91c8e37cabf73f735bd7cdda8ce24dbe0 Author: Jim Bosch <jbosch@astro.princeton.edu> Date: Wed Apr 16 17:41:55 2014 -0400   Ensure parent footprints include all their children   python/lsst/meas/deblender/deblend.py | 3 +++ 1 file changed, 3 insertions(+)  

Lauren MacArthur added a comment - 05/Mar/15 11:43 AM Oh, and I did force a Buildbot which indeed succeeded.

John Swinbank added a comment - 06/Mar/15 8:53 AM - edited Basically fine. A few comments: In afw , I find the comments a bit confusing. Footprint.h talks about extending a footprint to include its "children"; Footprint.cc just talks about a list of "others"; footprint1.py refers to "neighbors". To me, the term "children" implies there's some sort of hierarchical relationship among footprints, which, as far as I know, is not the case as far as afw is concerned (any parent/child hierarchy is introduced in meas_deblender , which is a higher level piece of code whose terminology we shouldn't adopt here). As far as I can tell, this code is quite generic – it will extend the footprint based on any list of others, regardless of their source, and it would be nice to make that explicit. In meas_deblender , I find the comment "update parent footprint in-place to ensure it contains all child footprints" not particularly useful – it basically says what the code is doing, which isn't hard to work out by reading the code, but gives no clue as to why it's necessary. Replacing it with something which explains what's going on would be nice. Finally, it would be good to include a test case in meas_deblender which demonstrates the problem and shows that it has been fixed. I realise that the lower-level code in afw is tested, but that's not where the bug actually occurred and this test doesn't actually establish that it's fixed.

Lauren MacArthur added a comment - 17/Mar/15 11:27 AM I have updated all comments as per your suggestions. I also wrote a unit test which will fail if this bug ever occurs again (in the context of this deblender algorithm). Forced Buildbot was successful. All updates are on branches u/lauren/ DM-1738 in afw and meas_deblender. afw: commit de65f62aca09357adaa7952610f62167077ede34 Author: Lauren MacArthur <lauren@astro.princeton.edu> Date: Mon Mar 16 14:12:56 2015 -0400   Clarify comments describing include function and homogenize variable names   include/lsst/afw/detection/Footprint.h | 7 ++++--- tests/footprint1.py | 4 +++- 2 files changed, 7 insertions(+), 4 deletions(-)   commit c839d0f28287875df3e43b674f1da3f140a33250 Author: Lauren MacArthur <lauren@astro.princeton.edu> Date: Mon Mar 16 16:34:35 2015 -0400   Update copyright and remove whitespace   include/lsst/afw/detection/Footprint.h | 12 ++++-------- src/detection/Footprint.cc | 2 +- tests/footprint1.py | 93 ++++++++++++++++++++++++++++++++++++--------------------------------------------------------- 3 files changed, 41 insertions(+), 66 deletions(-) meas_deblender: commit de65f62aca09357adaa7952610f62167077ede34 Author: Lauren MacArthur <lauren@astro.princeton.edu> Date: Mon Mar 16 14:12:56 2015 -0400   Clarify comments describing include function and homogenize variable names   include/lsst/afw/detection/Footprint.h | 7 ++++--- tests/footprint1.py | 4 +++- 2 files changed, 7 insertions(+), 4 deletions(-)   commit c839d0f28287875df3e43b674f1da3f140a33250 Author: Lauren MacArthur <lauren@astro.princeton.edu> Date: Mon Mar 16 16:34:35 2015 -0400   Update copyright and remove whitespace   include/lsst/afw/detection/Footprint.h | 12 ++++-------- src/detection/Footprint.cc | 2 +- tests/footprint1.py | 93 ++++++++++++++++++++++++++++++++++++--------------------------------------------------------- 3 files changed, 41 insertions(+), 66 deletions(-)   commit 1de2f16c1d0a1773950b5d7ec888e2e28f9822f4 Author: Lauren MacArthur <lauren@astro.princeton.edu> Date: Mon Mar 16 17:33:01 2015 -0400   Add unittest for DM-1738 Uses an image cutout that would suffer the replace-with-noise failure when the deblender results in child footprints that extend beyon the full extent of their parent's without the current fix. This test ensures that the footprint include method successfully expands a footprint to include the union of itself and all others provided.   tests/data/ticket1738.fits | 1892 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ tests/data/ticket1738_noInclude.fits | 1892 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ tests/data/ticket1738_noInclude.png | Bin 0 -> 126604 bytes tests/testInclude.py | 131 ++++++++ 4 files changed, 3915 insertions(+)

John Swinbank added a comment - 17/Mar/15 12:12 PM Looks good. See the comments on the meas_deblender PR for a few minor nits. Things that didn't fit there: Do you need to include the PNG file? Your test doesn't use it so far as I can see, and the content is (I assume) just the same as the FITS. Our guidelines suggest using no more than 50 characters for the first line of a commit message; you've exceeded this in a couple of places. Congratulations on your first unit test!

Lauren MacArthur added a comment - 17/Mar/15 12:53 PM Great! I addressed all you comments (but kept the 50+ chars in the commits as a trade off btw being concise and comprehensive). Merged and pushed to master.