Tim Jenness added a comment - 12/Sep/16 3:02 PM Just in case it's not obvious, if this is being added to lsst_distrib , please check that it works on Python 3 and also update the tests for pytest support.

Paul Price added a comment - 22/Sep/16 11:50 AM Nate Lust , would you mind reviewing this, please? There are a lot of changes to meas_extensions_convolved, but since these are changes from a C++-based measurement algorithm in HSC to a python-based measurement plugin in LSST, I suggest you review the current state. There are also changes to meas_extensions_photometryKron and meas_algorithms. price@price-laptop:~/LSST/meas/algorithms (tickets/DM-6784=) $ git sub commit 8859ba315f5d1b068d02f10dba3bb777cd716098 Author: Paul Price <price@astro.princeton.edu> Date: Wed Sep 21 16:27:37 2016 -0400   make aperture correction failures optionally non-fatal Some measurement algorithms may always fail to produce a result on certain data (e.g., seeing is too good or too bad to make the desired measurement). The failure to measure an aperture correction for such a measurement algorithm should not cause an exception. We now allow certain pre-specified measurement algorithms (through the new config parameter 'allowFailure') to fail and produce a warning rather than an exception.   python/lsst/meas/algorithms/measureApCorr.py | 19 +++++++++++++------ tests/testMeasureApCorr.py | 3 +++ 2 files changed, 16 insertions(+), 6 deletions(-)     price@price-laptop:~/LSST/meas/extensions/photometryKron (tickets/DM-6784=) $ git sub commit 4003c31208970ced5325aa55b96795a6c4252f69 Author: Paul Price <price@astro.princeton.edu> Date: Mon Jun 27 22:58:05 2016 -0400   make KronAperture public This will allow other modules to do Kron photometry.   include/lsst/meas/extensions/photometryKron.h | 67 +++++++++++++ .../lsst/meas/extensions/photometryKron/kronLib.i | 3 + src/KronPhotometry.cc | 107 +++++++-------------- 3 files changed, 103 insertions(+), 74 deletions(-)   commit f798a8a61639a3e727c33df62eb3e538394176b1 Author: Paul Price <price@astro.princeton.edu> Date: Thu Sep 15 16:30:01 2016 -0400   Remove unused config parameter 'fixed'   include/lsst/meas/extensions/photometryKron.h | 2 -- 1 file changed, 2 deletions(-)   commit 6e814988d11bc532f1a1dfc9448a38aedbe7ef27 Author: Paul Price <price@astro.princeton.edu> Date: Wed Sep 21 11:02:26 2016 -0400   store symbols for wrapped algorithm So external packages can use them without getting them from the registry.   python/lsst/meas/extensions/photometryKron/__init__.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)     price@price-laptop:~/LSST/meas/extensions/convolved (tickets/DM-6784=) $ git subcommit 2bc847650b92a466d2a6f072bd867fd1e547d19d Author: Paul Price <price@astro.princeton.edu> Date: Thu Sep 22 14:18:04 2016 -0400   convert to modern measurement framework This allows us to write the measurement algorithm ("plugin") in python, since all the heavy lifting is done using other packages. Deleted all the C++ code and replaced with python implementation that works with the modern measurement framework.   include/lsst/meas/extensions/convolved.h | 51 --- lib/SConscript | 3 - python/lsst/meas/extensions/convolved/SConscript | 3 - python/lsst/meas/extensions/convolved/__init__.py | 8 +- python/lsst/meas/extensions/convolved/convolved.py | 503 +++++++++++++++++++++ .../lsst/meas/extensions/convolved/convolvedLib.i | 51 --- src/convolved.cc | 353 --------------- 7 files changed, 505 insertions(+), 467 deletions(-)   commit abb0f388e8bd35f9cc236d2a9e1d7c6f60ac417e Author: Paul Price <price@astro.princeton.edu> Date: Thu Sep 22 14:20:34 2016 -0400   update and rename test The test has been updated to allow use with pytest, and renamed following the desired naming scheme. Also added tests for the Kron (previously ignored).   tests/convolved.py | 144 ------------------------------------- tests/testConvolved.py | 192 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 192 insertions(+), 144 deletions(-)   commit 0c7fb7df459242d027323b76ffee3a0896aaf57a Author: Paul Price <price@astro.princeton.edu> Date: Thu Sep 22 14:21:11 2016 -0400   .gitignore: add directory generated by test framework   .gitignore | 1 + 1 file changed, 1 insertion(+)

Tim Jenness added a comment - 22/Sep/16 1:02 PM Paul Price it won't build with lsstsw rebuild for me so can you please add it to the repos.yaml file (or I'm doing something wrong)? Also, it needs to be tested on Python 3 (see my earlier comment on Python 3 and pytest).

Paul Price added a comment - 22/Sep/16 1:18 PM Sorry Tim Jenness . I'm in the process of adding it to lsstsw, and then I'll test it with Python 3. Nate said he's not going to get to this for a few days, so this shouldn't block him.

Paul Price added a comment - 22/Sep/16 8:30 PM It builds under py3 . I removed one commit in meas_extensions_photometryKron that was ill-advised and breaking the build.

Nate Lust added a comment - 03/Oct/16 2:22 PM I made a few comments on some pull requests, some that require a bit of discussion, but nothing I think will be big show stoppers, after some discussion I will mark this reviewed.

Paul Price added a comment - 04/Oct/16 11:15 AM I've pushed some fixups commits for meas_extensions_photometryKron and meas_extensions_convolved that I believe address the issues raised on the GitHub PRs.

Nate Lust added a comment - 04/Oct/16 12:07 PM I am satisfied you addressed or considered all the comments, and provided build tests pass feel free to merge

Tim Jenness added a comment - 04/Oct/16 1:13 PM Please also add it to lsst_py3 meta package when done to ensure that we can check it on python 3.

Paul Price added a comment - 04/Oct/16 3:20 PM Jenkins passed under py2 and py3 . Rebasing away the fixups, and will then merge.

Paul Price added a comment - 04/Oct/16 3:37 PM Merged to master. Added package to lsst_py3. Thanks, all!

John Swinbank added a comment - 05/Oct/16 5:51 AM I don't see docs or a README in this package – surely it can't be marked as "done"?

John Swinbank added a comment - 05/Oct/16 5:54 AM DM-7896 .

Paul Price added a comment - 05/Oct/16 6:08 AM I don't see docs or a README in this package – surely it can't be marked as "done"? You must be new here....