Copying the review here:

Lauren MacArthur, on the MultiMatch constructor:

Should it be noted/documented somewhere that the default radius is 0.5 arcsec if None is provided?

Is 0.5 arcsec a bit generous for matching on astrometrically calibrated (although roughly) positions? Should this be set based on the estimated astrometric scatter? This may be moot as I think your rejection criteria account for any mismatches leaking in due to a large match radius, but restricting the match radius can shrink the initial number of "goodMatches", e.g.
========================================================================
radius = 0.5
Number of goodMatches = 89155
Number of safeMatches = 3468
RMS = 0.019516 magnitudes, with 0.49% (143 of 29362) measurements clipped as outliers.
========================================================================
radius = 0.21 # rougly 3*astrom scatter
Number of goodMatches = 64889
Number of safeMatches = 3518
RMS = 0.019513 magnitudes, with 0.49% (145 of 29811) measurements clipped as outliers.
========================================================================

Do you need to add radius = radius*lsst.afw.geom.arcseconds in the case where the call provides a float/int?

Define/describe idField and RecordClass in Arguments list. Indicate expected units for radius.

Is there a reason you didn't use the @param[in] format here?

Jim Bosch:

I'll document the default radius and I think I'll make it throw if you pass in a float, not an angle, just to avoid the case where someone passes in a float they think will be interpreted in some other unit.

Will add the other missing arguments to the docs too.

In pure Python code, I don't think we're required to use Doxygen markup, and Doxygen will handle it fine without it as long as we don't start the string with a !. I'm also aware that we'll be switching away from Doxygen to Sphinx in the near future (with yet another markup language), so this is probably actually a bit more future proof than using Doxygen markup right now.

Lauren MacArthur:

Copy that. But just to be clear, I was really wondering why you didn't use the @param[in] format here, but did use it for many functions below (e.g. build, aggregate, apply).

Jim Bosch:

Oh, I guess I didn't realize I was being inconsistent within a particular file. I don't really have a reason; it may have been code that began life as part of an IPython notebook vs. code that began life elsewhere.

If I'd seen this a moment earlier, I think I'd have fixed it because I do think consistency within a file is moderately important. But given that I've just merged it and it'll all be reformatted into restructed text soon enough, I'm just going to leave it.

Lauren MacArthur:

Would it be better to use, e.g., dataType instead of type here (since type is a python internal)?

Jim Bosch:

Will do.

Lauren MacArthur, on the class docs for GroupView:

Funny sentence construction here: "that provides an convenient operations on the"

Jim Bosch

Will remove the "an".

Jim Bosch Did you also see the comments for this commit:
https://github.com/lsst/afw/commit/f9bfe4543b8c75c342cca0ece11c8803a6dc3a81

Not a huge deal, but I do believe there is a typo in your notebook in box [13] (which likely filtered into Vishal Kasliwal [X]'s script).

This is the comment:

In [13] I think you meant get(modelMagKey) in:
modelMag = numpy.mean(cat.get(psfMagKey))

I ran this with a full focal plane run of the same visits. With the above fix the sigma went down slightly from 0.223 to 0.195. The difference in the number of safe matches was:
Old Number of safeMatches = 4024 # (before modelMagKey fix in notebook)
Number of safeMatches = 3468

Redundant call in [15]

Truncate sigma printing in [17]:
ax2.plot(gaussian, y, 'r-', label="Gaussian with sigma={0:0.4f}".format(sigma))

Maybe indicate the total number of measurements (before and after clipping) somewhere?

I saw that typo too and changed it in my version of Jim's script.

Lauren MacArthur thanks for the ping - I did not see that. I'll fix that on master on another issue.