Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: meas_extensions_photometryKron
-
Labels:
-
Story Points:3
-
Epic Link:
-
Sprint:Science Pipelines DM-W15-4, Science Pipelines DM-W15-5
-
Team:Data Release Production
Description
This is a low-priority ticket to replace the old-style plugins in meas_extensions_photometryKron with new ones compatible with meas_base. As this isn't a part of the main-line stack, we should delay it until other the meas_base conversion is nearly (or perhaps fully) complete.
Attachments
Issue Links
Activity
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.
moved this work onto a new branch, tickets/DM-982. u/pgee/DM-982 was abandoned, but all the versions of KronFluxAlgorithm.h and KronFluxAlgorithm.cc were moved to the new branch, so the review of that code should still apply. Since the changes were so substantial, merging the u/pgee/DM-982 with master was not feasible.
include/lsst/meas/extensions/photometryKron.h | 76 –
.../extensions/photometryKron/KronFluxAlgorithm.h | 143 ++++
.../meas/extensions/photometryKron/init.py | 3 +-
.../lsst/meas/extensions/photometryKron/kronLib.i | 34 +-
src/KronFluxAlgorithm.cc | 618 ++++++++++++++++
src/KronPhotometry.cc | 759 --------------------
tests/Kron.py | 163 +++--
ups/meas_extensions_photometryKron.cfg | 2 +-
ups/meas_extensions_photometryKron.table | 1 +
9 files changed, 883 insertions, 916 deletions
Please note that KronFluxAlgorithm.h and
KronFluxAlgorithm.cc are actually moved versions of KronPhotometry.h and KronShape.cc. I am now rather sorry I renamed them, since the git diff doesn't seem to know. If you have a fix for this, let me know
Perry Gee
added a comment - moved this work onto a new branch, tickets/ DM-982 . u/pgee/ DM-982 was abandoned, but all the versions of KronFluxAlgorithm.h and KronFluxAlgorithm.cc were moved to the new branch, so the review of that code should still apply. Since the changes were so substantial, merging the u/pgee/ DM-982 with master was not feasible.
include/lsst/meas/extensions/photometryKron.h | 76 –
.../extensions/photometryKron/KronFluxAlgorithm.h | 143 ++++
.../meas/extensions/photometryKron/ init .py | 3 +-
.../lsst/meas/extensions/photometryKron/kronLib.i | 34 +-
src/KronFluxAlgorithm.cc | 618 ++++++++++++++++
src/KronPhotometry.cc | 759 --------------------
tests/Kron.py | 163 +++--
ups/meas_extensions_photometryKron.cfg | 2 +-
ups/meas_extensions_photometryKron.table | 1 +
9 files changed, 883 insertions, 916 deletions
Please note that KronFluxAlgorithm.h and
KronFluxAlgorithm.cc are actually moved versions of KronPhotometry.h and KronShape.cc. I am now rather sorry I renamed them, since the git diff doesn't seem to know. If you have a fix for this, let me know 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.
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.