If it is going to go past the end of the Sprint, it doesn't really matter who does it. If you have time next week, that's fine. Otherwise, pass it back to Lauren.

Hi Perry –

I've added some review comments to this GitHub pull request. To summarize:

I am basically happy with the way you've ported the algorithm itself to the new framework. There are a few minor issues of code formatting which you should address before merging, and please make sure that any work which has to be performed in future is recorded in JIRA rather than marked as FIXME in the source.

I'm a bit more concerned about the state of the tests – it seems that there are quite a few checks which have been skipped without explanation, and a lot of duplication has been introduced. It may be that there are perfectly good reasons, but they aren't obvious from reading the code, but (similarly to the above) just commenting things out isn't a good answer – if it requires more work to make the tests pass, we should record what's required and why it can't be done now in JIRA.

As you note in the comment above, the tests fail if display=True. The correct fix is to run base_SkyCoord, as you suggest, but be aware that you need to add it to the single frame measurement (line 100 of tests/Kron.py) not the forced measurement (line 97).

As has been mentioned, you'll need to rebase your branch so that it has a logical & linear history before you can merge to master.

Finally, I'm a bit confused by the sequencing of the various issues here. According to the comments on this story, this work shouldn't have been done until the various HSC side fixes had been committed by Jim in DM-1783. However, it looks as though you branched from master before DM-1783 landed on 26 January. I don't think that invalidates your work here, but it might mean you have more work to do to merge your changes – certainly, your branch doesn't currently cleanly rebase onto master.

Review mostly complete; lots of small comments on the GitHub PR.

However, there's one larger issue that I didn't notice until looking at this code now: this conversion has dropped the special-case handling of forced photometry that previously existed here - we can't just use the SimpleAlgorithm implementation of the forced {measure()}} method, since that just delegates to the non-forced version. I think that should be a straightforward matter of resurrecting the old _applyForced method, and reworking that into an overload of the new version of measure(). But I could also imagine that it ends up being trickier than that, because the signatures of _applyForced and forced measure() aren't quite the same. I'm happy to consult on that, or even take it over if you'd like to devote your time to other issues as we close out this sprint. But I would like to get it fixed on this issue rather than a new one, so we don't disrupt the history by deleting the old implementation and then bringing in a new one that is (according to git) unrelated to it.

I believe I communicated to Perry off-line that I considered this review complete after I took a look at some of his changes since then, but I forgot to mark it complete here. Sorry for any confusion that may have caused. Perry Gee, you're good to merge once you're done with the changes we'd agreed upon.