Paul Price added a comment - 26/Aug/15 8:57 AM I propose to take Russell Owen 's comments as the review. Is this OK with you, Simon Krughoff ?

Paul Price added a comment - 26/Aug/15 9:23 AM I will update the matchOptimisticB API to require a Wcs . In _fitWcs , the wcs is updated for the next iteration (note that the call is in a loop). I'll modify the code to use Angle instead of implicitly assuming degrees.

Paul Price added a comment - 26/Aug/15 9:57 AM I've pushed two fixup commits for review: commit 6f741decada4b41983ba25d1ca17a12c47231897 Author: Paul Price <price@astro.princeton.edu> Date: Wed Aug 26 12:55:31 2015 -0400   fixup! matchOptimisticB: correct for distortion   include/lsst/meas/astrom/matchOptimisticB.h | 8 +++---- src/matchOptimisticB.cc | 36 +++++++++-------------------- 2 files changed, 15 insertions(+), 29 deletions(-)   commit c9ac6fecbe4d86ab9b8f305dfe2d62089b66fd4f Author: Paul Price <price@astro.princeton.edu> Date: Wed Aug 26 12:55:50 2015 -0400   fixup! matchOptimisticB: fix use of allowedNonperpDeg   src/matchOptimisticB.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)

Russell Owen added a comment - 26/Aug/15 10:56 AM Many thanks for the enhancements. Changing theta to an Angle was a good idea. I suspect other variables would benefit from the same treatment, but suggest that be left for a future cleanup pass (maybe one that also removes RecordProxy from the public namespace). One remaining request: please update the documentation of matchOptimisticB in matchOptimisticB.h. Presently it says that posRefCat reads "centroid" and "hasCentroid" fields, but that is no longer the case, thanks to your improvements. If you are willing, you could also add that the sourceCat fields read are "Centroid_x", "Centroid_y" and control.sourceFluxField.To aid that I provide the following suggested text, which I verified has a nicer format in HTML (significantly more so than the old text): @param[in] posRefCat catalog of position reference stars; fields read: - "coord" - control.refFluxField @param[in] sourceCat catalog of detected sources; fields read: - "Centroid_x" - "Centroid_y" - control.refFluxField I will also note a quirk of the Doxygen that I should have noticed before. I have no idea how to fix it and am not proposing you try. I'm just recording it. The documentation for the function "matchOptimisticB" is hard to find. The link of that name under the tab "Namespaces: Namespace List" points to documentation of the python module of that name, not the C++ function. I found documentation for the C++ function under "Files:matchOptimisticB.h". Also also found it under the tab "Namespaces: Namespace Members", but that page is really cluttered and clumsy to use and the linked entry includes an unwanted entry for the function from the .cc file, even though the .cc file contains no doxygen comments.

Paul Price added a comment - 26/Aug/15 11:18 AM Thanks Russell. I updated the doc, rebased and merged to master.