These are the files I propose to remove from meas_algorithms. With some changes to a few files (PsfAttributes.cc, psfImage.cc, secondMomentStarSelector,py, SincFlux.cc, and several quick tests) I am able to get the remainder to compile with what is left. I have not yet tried to compile the rest of the stack.

nclude/lsst/meas/algorithms/Algorithm.h
include/lsst/meas/algorithms/ApertureFlux.h
include/lsst/meas/algorithms/CentroidControl.h
include/lsst/meas/algorithms/Classification.h
include/lsst/meas/algorithms/CorrectFluxes.h
include/lsst/meas/algorithms/FluxControl.h
include/lsst/meas/algorithms/GaussianFluxControl.h
include/lsst/meas/algorithms/Measure.h
include/lsst/meas/algorithms/PixelFlags.h
include/lsst/meas/algorithms/RecordCentroid.h
include/lsst/meas/algorithms/ScaledFlux.h
include/lsst/meas/algorithms/SdssShapeControl.h
include/lsst/meas/algorithms/ShapeControl.h
include/lsst/meas/algorithms/SkyCoord.h
include/lsst/meas/algorithms/detail/SdssShape.h

src/Classification.cc
src/CorrectFluxes.cc
src/Measure.cc
src/PixelFlags.cc
src/SkyCoord.cc
src/flux/ApertureFlux.cc
src/flux/EllipticalApertureFlux.cc`
src/flux/GaussianFlux.cc
src/flux/NaiveFlux.cc
src/flux/PeakLikelihoodFlux.cc
src/flux/PsfFlux.cc
src/centroid/GaussianCentroid.cc
src/centroid/NaiveCentroid.cc
src/centroid/RecordCentroid.cc
src/centroid/SdssCentroid.cc
src/centroid/twodg.cc
src/centroid/all.h

python/lsst/meas/algorithms/algorithmRegistry.py
python/lsst/meas/algorithms/measurement.py

tests/testsCorrectFluxes.py
tests/photometry.cc
test/photometryWithoutPsf.py
tests/sincPhotSums.py

Can we get rid of PsfAttributes too?

Maybe. What is the reasonable replacement for getGaussianWidth?

I don't see a lot of clients which use this class, though I haven't tried compiling without it.

Just a few recommended changes:

• I think we might want to keep examples/measAlgTasks.py, and update it to use SingleFrameMeasurementTask, as I think it may be an important part of the documentation for SourceDeblendTask.
• We should get rid of detail/SincPhotometry.h and Photometry.h, as those are definitely duplicated in meas_base already.
• I think we probably want to move tests/sincPhotSums.py to meas_base and modify it as necessary to use new meas_base interfaces, as we don't have low-level test coverage like this for the sinc photometry algorithms there.
• We should be able to get rid of python/lsst/meas/algorithms/measureSourceUtils.py (given what's in it, it should be trivial to fix any code that depends on it).

As for PSFAttributes, I think it's probably most appropriate to leave its removal to another ticket, just because fixing the few remaining bits of code that use it could end up being a rabbit hole that would delay the rest of this work, and we should probably add an effective area method to Psf as part of replacing it. I'll create a new issue to do that.

I see from buildBot that the C++ test, testImagePsf, is failing. I will check into that tomorrow. Since I change the ImagePsf.cc file, I suppose that I am not returning something correctly.

I'm puzzled, because testImagePsf passes on my machine, but not on buildBot. I think I checked everything in, but obviously I missed something.

I think that the review can be done while I look for this problem, but you can also wait until I find the problem if you prefer.

The problem with ImagePsf seems to be caused by a legitimate failure in the computeSincFlux method of ApertureFlux which apparently did not appear the the comparable code in meas_algorithms. The test uses a 25x25 pixel Psf image, and progressively uses a radius of 3, 5, 10, and 20. At 5 and 10, the SINC_COEFFS_TRUNCATED flag gets set, but the measurement is made. At 20, the APERTURE_TRUNCATED flag is set, and the measurement returns NAN.

This was an intentional change by me; the old sinc photometry code didn't correctly report when it wasn't necessarily doing a good job. The unit test should be modified to reflect the new behavior.

Here are my exceptions to the tests and examples you've removed:

• examples/measAlgTasks.py: should be modified to use SingleFrameMeasurement, but should remain in meas_algorithms (since it also uses SourceDetectionTask).
• I think examples/growthcurve.py should be modified and moved to meas_base, as it's useful example code for the aperture photometry algorithms.
• tests/sincPhotSums.py should be modified and moved to meas_base, where I believe we still have a very similar interface in the SincCoeffs class that currently lacks test code.

Moving files from one git repo to another while preserving history is indeed a pain, but you can find step-by-step instructions on StackOverflow. Don't worry about not understanding all the steps (I've used them before, and I don't understand them); just follow them carefully and check that the results are what you expect.

There's a debug print line it looks like you forgot to remove on PsfAttributes.cc:319

Aside from the above, all the changes you've made look fine, but there's one more category of changes I think should be done here: it's time to remove all the version 0 workarounds and conditional code from meas_base and pipe_tasks (and maybe some obs_* packages?), since it's now no longer possible to run the old measurement task.

I don't think that measAlgTasks is worth the bother. It you disagree, I can transform it (already done). I'm just not sure how to revert the single file deletion on my branch, now that it is checked in to u/pgee/DM-420. The others were correctly deleted, since they were moved to meas_base.

measAlgTasks is used as the example in the task documentation for SourceDetectionTask, so I think it's actually the only one of these that is absolutely necessary to preserve.

To get it back, you can do a "git rebase -i", then use the "edit" command for the commit that deletes it. At that point, you can do "git reset --soft HEAD^" (undoes that commit but leaves all the changes in place), remove the deletion from the staging area, and then do that commit again.

Jim, could you mark this Review Complete. If you want to look at it first, u/pgee/DM-420 in meas_base and meas_algorithms.

Be careful to get a new clone.

Most of my complaints here are on the git history, so I'm just going to put all of my comments here instead of on a GitHub PR. And I'm afraid I'm not quite ready to sign off on this yet; there are just too many trivial errors that should have been dealt with before this went out to review again, including some that I noted in my earlier review. On that note, I know you prefer to do the git history cleanup after the review (so I understand if many of the issues here were things you'd already planned to fix), but after this issue I'm convinced that's not really viable - it just makes the reviewer spend too much time commenting on things that the branch author might already be planning to fix, because there's no way of knowing what the branch author intended to fix at that point.

The git history in meas_base needs quite a bit of cleanup:

• You've got a merge from master in here, which should be replaced by a rebase on master (if you just do a "git rebase master", I think that should fix it).
• There's something weird going on with the new code in tests.py; you add it in one commit, remove it from the next, and then add it back in in the last (non-merge) one. From the commit messages, it looks like it should just be added in the first one.
• There's code in the first commit dealing with GaussianCentroid that looks like it belongs in the second one.

The comments and indentation around the new method in GaussianCentroid are a mess; we don't want that long line of asterisks running through the middle of the class, there are weird spaces in the rest of the comment, and the close paren is in the wrong place. And one of the lines is longer than 110 characters.

I mentioned in my earlier review that this was the time to remove the version 0 and workaround code. I haven't done a thorough search of what that entails myself, but it should include at least the Version0FlagMapper function and the _flagMap dict in base.py in meas_base.

The git history in meas_algorithms needs cleanup:

• There's another merge-from-master commit there that needs to go.
• Typos in the commit message for the transfer of sincPhotSums

There is a debug `writeFits` statement in `PsfAttributes.cc`, as well as some debug printing.

You've commented out some code around lines 43 of measAlgTasks.py; it looks like the original author put it there for a reason, so I'd like to see it uncommented. You've also added a "display=True" line that I suspect should be reverted.

Line 38 of ImagePsf.cc is commented out, but should just be removed.

I'm rather confused by some of your comments, as I took some of these problems out before I checked in. I did have problems show up after I tried to fix the file deletions that you objected to, and problems are now showing up in code which I thought I had fixed. But for whatever reason, I am finding this regimen hard to adapt to, and it isn't actually intentional.

Hmm, ok, let me know if I can help sort it out. Just to double-check, the last commits I see are 83d49c8 in meas_algorithms and 1519eb55 in meas_base.

And I certainly understand that the switch to a stricter git commit policy is tough to get used to, and it's probably been extra frustrating on this issue with the effort to transfer file history that ended up going nowhere.

Thanks for being understanding. I see that there is pressure to make all these commits perfect, and I must admit to being pretty frustrated when I put a lot of effort into it and it still seems to get screwed up. But I am also beginning to wonder whether I can adjust to a regimen where the little details are so important. I am used to worrying about whether the code does the right thing. Still, I have been trying.

I had to completely rebuild this checkin almost from scratch, since the merge to master in the middle of it was making it impossible to squash the commits together. I think in the future, I will utilize a separate working branch, then move to a checkin branch to avoid all this trouble. I still don't know how some of my changes got destroyed in the process of rebasing. Probably pilot error, but rebasing in the presence of lots of change from other people on the same repo is easy to get screwed up.

Do not that a merge to master, not a rebase master, was the suggested way of doing things for the longest time. And merges really seem to cause problems when trying to tidy up a commit history.

I did remove the tableVersion and Version0Flag stuff, although I was not completely sure that we were ready to do this. When I initially tackled this issue, I judged it too soon, as I was concerned about files sitting around on disk which people might want to access with various tasks. I have now pulled all of that out except for code in meas_astrom, which still clearly depends on files which are still sitting around in astrometry_net_data directories, and which we have not evolved a plan to replace. So I moved the field mapping code to meas_astrom, so it now only exists there.

Removing this code from meas_base and tableVersion from pipe_tasks cascaded changes to a number of repos. so that we now have the following u/pgee/DM-420 branches:

meas_base
meas_algorithms
meas_astrom
meas_deblender
pipe_tasks
obs_sdss

Just to warn the reviewer, the buildBot run on u/pgee/DM-420 broke pex_policy in the doc part of the build, for no reason that I can particularly figure out.

Dictionary.cc, where the break occurs, has not been altered in some time.

The list of packages with u/pgee/DM-420 branches that I see doesn't match the list you gave me. Could you make sure you've pushed everything, and send me the SHA1s of the tips? Here's my list of packages with u/pgee/DM-420 branches:

• meas_algorithms: 6911a59d
• meas_astrom: ae89fd51
• meas_deblender: 22f14a7e
• pipe_tasks: da4f9590
• meas_extensions_photometryKron: 694451bb
• meas_extensions_shapeHSM: b45f56ba

I think you can probably ignore the BuildBot failure, as it's almost certainly one of its many intermittent failure modes - but if you can try submitting that build again to check that, it'd be nice to make sure.

I completely agree that merging from master to a ticket branch is a big problem for cleaning up the history, and I think it's a shame that was ever the recommended procedure (I think there was too much fear of rebase early on). Unfortunately, it does seem like merging from master to the ticket branch is at least implied as a recommended procedure here; I suspect RFC-21 includes fixing that, but I'll make sure.

Hmm, I now see a meas_base branch at c369d3e3; unless you've pushed it since my last post, maybe it was just user error on my part. Would still appreciate a clarification on the status of the branches for the meas_extensions packages, but if I don't hear from you I'll assume they're part of the work and you just forgot to list them above.

meas_base c369de
meas_algorithms c76871
pipe_tasks e0116

These three were all rebased, so that might be the problem. The SHA1s are not in my histories, so you might need a new clone.

Note that I just did another push -f on pipe_tasks as well.

The others you listed match. I forgot about the meas_extensions_*, which were done during buildBot a couple of weeks ago. How did you find these (without looking through all the repos)?

Yes, the meas_extensions are part of the ticket. I made these changes on lsst-dev, and never pulled them to my working machine. This is one of the gotchas about git local repos. Useful, but potentially confusing if you don't have the habit of fetching constantly.

Thanks for the clarifications. I'll make sure I get new versions of all of those.

I found the meas_extensions_* branches by using my bot tool, but I also just noticed you can find them on this issue page, where it shows the git branches and pull requests associated with this issue.

Review complete. I've made some minor comments on PRs for meas_base and meas_algorithms, but I think all of those will be easy changes to make, so there's no need to check back with me before merging (unless you want to).

Other packages look fine, so I didn't even make PRs for them (aside meas_astrom, where I just said that things were fine).

However, I wonder if we can now remove the dependencies on meas_algorithms from the meas_extensions .table and .cfg files. Would you mind trying that and seeing if they still get through buildbot?

I swear that I made those debugging and cosmetic changes to measAlgs.py, PsfAttributes.cc, and ImagePsf.cc at least once before. The display=True for sure. Sorry I left that junk in.

I am not able to remove the include lsst/meas/algorithms.h from hsmLib.i without some work, so I am going to skip that, and go ahead and run buildBot on the rest. It is probably something simple, but it is not obvious to me on cursory inspection. If you can spot it, send me email. I will probably wait until later the merge, when the buildBot is done.

meas_extensions_photometryKron seems to be happy with the idea.

The comment you made in ImagePsf about the separate declaration of the declaration of the const & from the _psfImage: I was getting a type error on the call to fitCentroid which had me convinced that I had to create a const & (or create in the call, which was pretty verbose looking).
I am surprised to find that your suggestion compiles.