Additional information would be very helpful. Was the aperture correction omitted by simply failing to port certain commits which could then be cherry-picked?

KronPhotometry.cc seems to be the main implementation file, and comparing the master on HSC and LSST shows that the two files have diverged significantly. However, the only algorithmic difference I can find is as follows:

• LSST's KronFluxAlgorithm::measure ignores the variable axes computed earlier and computes a new one using (unfortunately with the same name) using afw::geom::ellipses::Axes axes(source.getShape()); just before using it to compute aperture. The HSC equivalent code (KronFlux::_apply) uses the value of axes computed earlier. This appears be a misfeature in the LSST code: the code that computes the earlier value of axes doesn't seem to have any net effect and the code is needlessly confusing.

The only other changes I found, beyond simple mechanical changes that were obviously made to adapt to the new measurement framework, afw table and exception names, are as follows:

• LSST's code adds KronFluxAlgorithm::measureForced, which has no equivalent on the HSC side.
• HSC's code is templated on pixel type; LSST requires float. Perhaps this change was made due to the new measurement framework.

Perry Gee, this issue was originally filed at your request (DM-1953). Could you please help Russell out?

I can't honestly explain why the axes constructor was put into this code. It is probably a mistake on my part, though axes should be defined before we arrive at that line, so I can't thing of why I would have defined it again.

It looks like the code is essentially a no-op with the default config, unless getShapeFlag() is true, in which case, the new code looks like it would cause a failure. So I suggest you take the line out. This would have created a particularly nasty bug, as it probably isn't detected in the unit tests.

Did you figure out what the issues were with aperture correction not being available? I think I originally left some comment lines in about that, but they were taken out during review.

Perry: I will need more information to add aperture correction to this code.

I did compare the LSST code code to HSC, hoping HSC had aperture correction. However, the only significant difference is the bug you describe in your comment.

Meanwhile I will use this ticket to fix the bug, as per your suggestion.

Note that the validation in tests/Kron.py has (long!) been disabled:

        #

        # Choose function that does the measuring

        #

        if False:                       # testing only; requires XY0 == (0, 0)

            measureKron = self.measureKronInPython

        else:

            measureKron = self.measureKron

This might have uncovered the axes issue.

On master (with no changes) the unit test tests/Kron.py fails for me with this error:

lsst::pex::exceptions::NotFoundError: 'Field or subfield withname 'base_CircularApertureFlux_12_0_flux' not found with type 'D'.'

I also observe that the test requires meas_algorithms but it is not in the ups table file. I will it add that as an optional dependency.

I suspect that test failure means the package should have been updated for DM-3108, but it wasn't because it's not included in CI. If that's something you don't want to handle on this ticket, open a new one and assign it to me (but I'm unlikely to have chance to look before Bremerton, sorry).

Thanks, John. The ticket number that caused the breakage was the hint I needed. I'll fix the problem on this ticket.

Paul: for the record, I think the test you commented on runs the C++ code in its normal mode, and so is correct. If one changes "if False:" to "if True:" where you indicated then I believe it runs a Python implementation instead of the C++ code. That breaks the test, which is unsurprising because that method is documented to require XY0 = 0,0, which is not the case for the test. It could use some cleanup, such as making the python implementation work with XY0 != 0,0 if it's wanted, or ditching it if not.

I spent a few minutes this morning trying to figure out what's required here. I think this is the deal, but please do correct me if I've got the wrong end of the stick.

When working DM-982 ("convert meas_extensions_photometryKron to new measurement framework"), Perry Gee came across some lines of the form msConfig.algorithms.names.remove("correctfluxes") which had previously been added by Jim Bosch to "remove now-implicit aperture correction from test code". The correctfluxes algorithm didn't exist in the meas_base framework, so this caused things to break; he worked around the issue by commenting those lines out. In code review, both Jim & I pulled him up on this – it's not right to leave commented-out code lying around – and he removed it, but protested that it would need to be re-added once aperture corrections were implemented in meas_base (for the same reason it was originally added). Hence this issue.

I've not become properly familiar with the new aperture correction system, but my understanding is that it does not work through an additional algorithm à la correctfluxes. There's therefore no direct equivalent of the original code that should be enabled.

Instead, aperture corrections are enabled by passing an appropriate argument to lsst.meas.base.wrapSimpleAlgorithm. That's called in photometryKron's __init__.py. Currently, aperture corrections are disabled; naively, I would hope that simply providing an appropriate bool there is all that's required to enable them.

Exposing my ignorance: I do not know whether it is appropriate to enable aperture corrections here. I can't parse Jim's original commit to be sure if the aperture corrections are supposed to be implicit in the Kron photometry code or just in the particular test cases in which he explicitly disabled them, and I don't have time to dive in to find out at the moment. Perhaps Jim or Perry can clarify; once that's established, I think this issue becomes effectively trivial.

So: comments? Have I understood everything correctly? And apologies to Russell Owen for a particularly ill-described story.

I think you've understood everything correctly; it should just be a matter of setting that bool in the call to wrapSimpleAlgorithm.

I suppose I didn't completely answer the question. We do want to enable aperture corrections on the Kron magnitudes, since that's what we've done on the HSC side, even though the aperture corrections aren't really appropriate for galaxy fluxes; the argument is that applying stellar aperture corrections to extended source fluxes is still better than applying none at all.

Thanks. I should have seen that. A trivial change indeed. Unfortunately the unit test passes with or without aperture correction enabled. I'll see if I can fix that before putting this up for review.

Another task that may be out of scope for this ticket is to get rid of the "if False:" in the unit test, or fix the python implementation so it can handle non-zero xy0 and run both implementations, if that makes any sense. I have no idea why the python implementation even exists (nor why it requires non-zero xy0 – much of it is written using the current xy0, so that limitation suggests bug, possibly one that is easy to fix). I would welcome opinions on what to do about it on this ticket.

I expect the python implementation exists for validation of the C++ code — it's a different code used to measure the same thing, so we can have more confidence the C++ code is doing it right. If it was active, it may have caught the axes issue.

I am going to suggest we move improvement of the unit test to a new ticket.

• If the python implementation is indeed wanted then it needs work to make it compatible with nonzero xy0 and then it needs to be enabled and used. It would be easier to fix this if Perry told us what he had in mind for the code (though Paul's guess sounds reasonable).
• The current unit test doesn't appear to run enough code to generate aperture correction data, so we cannot test the effects of aperture correction. I suspect fixing this will take significant effort.

If others agree then I'll file a new ticket and ask someone to review this one.

I'm certainly happy with not fixing the Python implementation which is used (or, rather, disabled) in the test suite: that seems completely out of scope here.

It would be nice to extend the test to demonstrate the effects of aperture correction. However, that doesn't seem like a high priority: if you judge that it's a lot of work, I have no problem with shunting it to a new ticket and working on something more urgent instead.

If we do test aperture correction here at all (and I'm not saying we need to, at least on this issue), I think it's sufficient to merely test their application, not their measurement (since the latter requires doing quite a bit more and is tested elsewhere). In other words, we could simply mock up an ApCorrMap with a constant ChebyshevBoundedField and verify that running measurement with aperture correction enabled scales the expected result by the value in the ChebyshevBoundedField.

Jim: that is an excellent suggestion. I'll give that a try. It will add some time to the ticket, but is worth doing.

Lauren: do you have time to look at this?

All the work is on meas_extensions_photometryKron tickets/DM-2429

I was able to add a test for aperture correction to the existing unit test.

It looks like you should squash these two commits:
54d4327 2015-08-14 Add a test that aperture correction works. [Russell Owen]
768f5a7 2015-08-14 starting to add ap corr to unit test [Russell Owen]

Otherwise, I built your branch and ran processCcd.py on HSC data and all looks ok within the context of this ticket (per Jim's comment above).

After squashing, feel free to merge to master.

Thank you for catching the missing squash. I did that and merged to master.