Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-982

convert meas_extensions_photometryKron to new measurement framework

    XMLWordPrintable

    Details

      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

            No builds found.
            jbosch Jim Bosch created issue -
            jbosch Jim Bosch made changes -
            Field Original Value New Value
            Epic Link DM-16 [ 11264 ]
            jbosch Jim Bosch made changes -
            Rank Ranked higher
            jbosch Jim Bosch made changes -
            Rank Ranked lower
            jbosch Jim Bosch made changes -
            Rank Ranked higher
            jbosch Jim Bosch made changes -
            Rank Ranked higher
            jbosch Jim Bosch made changes -
            Epic Link DM-16 [ 11264 ] DM-1100 [ 13907 ]
            jbosch Jim Bosch made changes -
            Labels Measurement AppsPlanning
            jbosch Jim Bosch made changes -
            Story Points 2 3
            jbosch Jim Bosch made changes -
            Story Points 3 5
            jbosch Jim Bosch made changes -
            Labels AppsPlanning SciencePipelines
            swinbank John Swinbank made changes -
            Sprint Science Pipelines DM-W15-3 [ 111 ]
            swinbank John Swinbank made changes -
            Rank Ranked lower
            swinbank John Swinbank made changes -
            Sprint Science Pipelines DM-W15-3 [ 111 ]
            swinbank John Swinbank made changes -
            Rank Ranked higher
            Hide
            price Paul Price added a comment -

            There has been a lot of work recently on meas_extensions_photometryKron on the HSC side. This should be pulled over before refactoring; otherwise this effort will be difficult to merge back.

            Show
            price Paul Price added a comment - There has been a lot of work recently on meas_extensions_photometryKron on the HSC side. This should be pulled over before refactoring; otherwise this effort will be difficult to merge back.
            jbosch Jim Bosch made changes -
            Sprint Science Pipelines DM-W15-4 [ 120 ]
            jbosch Jim Bosch made changes -
            Rank Ranked higher
            jbosch Jim Bosch made changes -
            Story Points 5 3
            jbosch Jim Bosch made changes -
            Link This issue is blocked by DM-1783 [ DM-1783 ]
            jbosch Jim Bosch made changes -
            Rank Ranked higher
            Hide
            jbosch Jim Bosch added a comment -

            Perry Gee, as you've probably seen, I just merged DM-1783, which should address Paul Price's concern above and hence let you convert this algorithm to the meas_base framework without making it difficult to transfer patches between LSST and HSC. I'll do the same for shapeHSM later this week.

            Show
            jbosch Jim Bosch added a comment - Perry Gee , as you've probably seen, I just merged DM-1783 , which should address Paul Price 's concern above and hence let you convert this algorithm to the meas_base framework without making it difficult to transfer patches between LSST and HSC. I'll do the same for shapeHSM later this week.
            pgee Perry Gee made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            Hide
            pgee Perry Gee added a comment -

            Jim: did you disable the unit test Kron.py? It would be nice to run it under meas_algorithms for comparison purposes.

            Show
            pgee Perry Gee added a comment - Jim: did you disable the unit test Kron.py? It would be nice to run it under meas_algorithms for comparison purposes.
            Hide
            jbosch Jim Bosch added a comment -

            I disabled it, but I hoped you would be able to re-enable at least most of it on this issue. The reason I had to disable it was because it assumed a forced photometry API in the old framework which never existed on the LSST side. I may have been overzealous in disabling it though - you might be able to re-enable most of it while still avoiding that problem.

            Show
            jbosch Jim Bosch added a comment - I disabled it, but I hoped you would be able to re-enable at least most of it on this issue. The reason I had to disable it was because it assumed a forced photometry API in the old framework which never existed on the LSST side. I may have been overzealous in disabling it though - you might be able to re-enable most of it while still avoiding that problem.
            Hide
            pgee Perry Gee added a comment -

            Please review this conversion of KronFlux to meas_base framework. Note that there is a rename here which seems to show up as a lot of changes. I just renamed the h and cc files, but git doesn't seem to be that smart.

            include/lsst/meas/extensions/photometryKron.h | 76 –
            .../extensions/photometryKron/KronFluxAlgorithm.h | 130 ++++
            .../meas/extensions/photometryKron/_init_.py | 20 +-
            .../lsst/meas/extensions/photometryKron/kronLib.i | 39 +-
            src/KronFluxAlgorithm.cc | 560 +++++++++++++++
            src/KronPhotometry.cc | 759 --------------------
            tests/Kron.py | 303 ++++----
            ups/meas_extensions_photometryKron.cfg | 2 +-
            ups/meas_extensions_photometryKron.table | 1 +
            9 files changed, 873 insertions, 1017 deletions

            Show
            pgee Perry Gee added a comment - Please review this conversion of KronFlux to meas_base framework. Note that there is a rename here which seems to show up as a lot of changes. I just renamed the h and cc files, but git doesn't seem to be that smart. include/lsst/meas/extensions/photometryKron.h | 76 – .../extensions/photometryKron/KronFluxAlgorithm.h | 130 ++++ .../meas/extensions/photometryKron/_ init _.py | 20 +- .../lsst/meas/extensions/photometryKron/kronLib.i | 39 +- src/KronFluxAlgorithm.cc | 560 +++++++++++++++ src/KronPhotometry.cc | 759 -------------------- tests/Kron.py | 303 ++++---- ups/meas_extensions_photometryKron.cfg | 2 +- ups/meas_extensions_photometryKron.table | 1 + 9 files changed, 873 insertions , 1017 deletions
            pgee Perry Gee made changes -
            Reviewers Lauren MacArthur [ lauren ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            Hide
            pgee Perry Gee added a comment -

            Lauren, will you have time to review this?

            Show
            pgee Perry Gee added a comment - Lauren, will you have time to review this?
            Hide
            lauren Lauren MacArthur added a comment -

            Perry Gee. I started looking at this on Friday, but ran out of time. Two quick initial comments:
            1) There are many warnings when building about hiding a function measure().
            2) The unit tests fail when display=True (I noted this too late in the day to look into the details)

            Show
            lauren Lauren MacArthur added a comment - Perry Gee . I started looking at this on Friday, but ran out of time. Two quick initial comments: 1) There are many warnings when building about hiding a function measure(). 2) The unit tests fail when display=True (I noted this too late in the day to look into the details)
            Hide
            pgee Perry Gee added a comment -

            Curious, I don't get these warnings. Do you have the equipment at the conference to show this to me?

            Show
            pgee Perry Gee added a comment - Curious, I don't get these warnings. Do you have the equipment at the conference to show this to me?
            Hide
            pgee Perry Gee added a comment -

            The warnings apparently have to do with Clang. I assume we will deal with these on another ticket, since they are not unique to DM-982.

            The fix to the display problem is to add "base_SkyCoord" to the list of algorithm names on line 97. This problem only appears when the display option is used, so it is not encountered by the unit tests.

            Lauren was concerned that my commits were not logical according to the rules as she understood them. This is frequently true when I am on my own branch. This gets corrected during the rehash -i before checking in, but didn't seem necessary beforehand, at least in simple cases.

            That's one of the reasons that I always include the log of diffs between the original checkout on the branch and the thing I want to get reviewed. Normally, there is logically only one commit in an issue, and that is the commit (which eventually will get squashed into one) corresponding to the difference between the branch origin and the current state of the branch.

            John, if you want me to squash before review, I can do so. But it doesn't seem necessary with simple changes like this one. If there is an independent commit inside, I will try to note it in a comment. Perhaps we can talk about this at our first standup to establish best practice.

            Show
            pgee Perry Gee added a comment - The warnings apparently have to do with Clang. I assume we will deal with these on another ticket, since they are not unique to DM-982 . The fix to the display problem is to add "base_SkyCoord" to the list of algorithm names on line 97. This problem only appears when the display option is used, so it is not encountered by the unit tests. Lauren was concerned that my commits were not logical according to the rules as she understood them. This is frequently true when I am on my own branch. This gets corrected during the rehash -i before checking in, but didn't seem necessary beforehand, at least in simple cases. That's one of the reasons that I always include the log of diffs between the original checkout on the branch and the thing I want to get reviewed. Normally, there is logically only one commit in an issue, and that is the commit (which eventually will get squashed into one) corresponding to the difference between the branch origin and the current state of the branch. John, if you want me to squash before review, I can do so. But it doesn't seem necessary with simple changes like this one. If there is an independent commit inside, I will try to note it in a comment. Perhaps we can talk about this at our first standup to establish best practice.
            pgee Perry Gee made changes -
            Reviewers Lauren MacArthur [ lauren ] John Swinbank [ swinbank ]
            Hide
            pgee Perry Gee added a comment -

            BTW, I did not push the fix to Kron.py test because I could not figure out how to push to the new repo. Something missing in Frossie's instructions? However, the fix to the display problem is a one liner, and is described above.

            Show
            pgee Perry Gee added a comment - BTW, I did not push the fix to Kron.py test because I could not figure out how to push to the new repo. Something missing in Frossie's instructions? However, the fix to the display problem is a one liner, and is described above.
            Hide
            swinbank John Swinbank added a comment -

            Hi Perry - I'm happy to look at this, but probably won't have time now until mid-next-week.

            I've not looked at your code yet at all, but I'm happy enough to review the overall change without you rebasing it to a logical commit structure. If it's a problem though, you will need to fix it before merging to master.

            Show
            swinbank John Swinbank added a comment - Hi Perry - I'm happy to look at this, but probably won't have time now until mid-next-week. I've not looked at your code yet at all, but I'm happy enough to review the overall change without you rebasing it to a logical commit structure. If it's a problem though, you will need to fix it before merging to master.
            Hide
            pgee Perry Gee added a comment -

            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.

            Show
            pgee Perry Gee added a comment - 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.
            swinbank John Swinbank made changes -
            Sprint Science Pipelines DM-W15-4 [ 120 ] Science Pipelines DM-W15-4, Science Pipelines DM-W15-5 [ 120, 129 ]
            swinbank John Swinbank made changes -
            Rank Ranked higher
            Hide
            swinbank John Swinbank added a comment - - edited

            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.

            Show
            swinbank John Swinbank added a comment - - edited 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.
            swinbank John Swinbank made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            swinbank John Swinbank made changes -
            Link This issue has to be done before DM-2131 [ DM-2131 ]
            Hide
            pgee 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

            Show
            pgee 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
            pgee Perry Gee made changes -
            Reviewers John Swinbank [ swinbank ] Jim Bosch [ jbosch ]
            Status Reviewed [ 10101 ] In Review [ 10004 ]
            Hide
            jbosch Jim Bosch added a comment - - edited

            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.

            Show
            jbosch Jim Bosch added a comment - - edited 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.
            Hide
            jbosch Jim Bosch added a comment -

            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.

            Show
            jbosch Jim Bosch added a comment - 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.
            jbosch Jim Bosch made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            pgee Perry Gee made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            frossie Frossie Economou made changes -
            Team Measurement Framework [ 10205 ] Data Release Production [ 10301 ]
            swinbank John Swinbank made changes -
            Component/s meas_extensions_photometryKron [ 12318 ]
            Component/s meas_extensions_shapeHSM [ 10740 ]

              People

              Assignee:
              pgee Perry Gee
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              Jim Bosch
              Watchers:
              Jim Bosch, John Swinbank, Lauren MacArthur, Paul Price, Perry Gee
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.