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

Test four algorithms for compatibility with original meas_algorithms

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Labels:

      Description

      Do a trial run of a small area of sky (using a single exposure from measMosaicData). First create a source catalog using the old measurement.py, then run the same test with the measurement task in sfm.py. Compare the results. If the code has been ported correctly, they should match.

        Attachments

          Activity

          Hide
          pgee Perry Gee added a comment -

          The goal of this exercise is to run the newly minted algorithms on real (well, lsstSim) data, and see how the results match up with the existing meas_algorithm framework

          Show
          pgee Perry Gee added a comment - The goal of this exercise is to run the newly minted algorithms on real (well, lsstSim) data, and see how the results match up with the existing meas_algorithm framework
          Hide
          pgee Perry Gee added a comment -

          This is the code fixes which came out of this review. I will put the results of the review on a wiki page once the PsfFlux bugs are fixed.

          tickets/DM-445 in meas_base
          [pgee@lsst-dev mb5]$ git diff --stat master
          include/lsst/meas/base/SdssCentroid.h | 3 ++-
          include/lsst/meas/base/SdssShape.h | 4 ++--
          python/lsst/meas/base/plugins.py | 2 +-
          src/Inputs.cc | 2 +-
          src/SdssShape.cc | 16 +++++++---------
          tests/testClassificationBasic.py | 2 +-
          6 files changed, 14 insertions, 15 deletions

          Add setVersion to the creation of the measurement table:
          [pgee@lsst-dev pipe_tasks]$ git diff --stat master
          python/lsst/pipe/tasks/processImage.py | 2 ++
          1 files changed, 2 insertions, 0 deletions

          Changes to Source.h in u/pgee/s14
          [pgee@lsst-dev afw4]$ git diff --stat master
          include/lsst/afw/table/Source.h.m4 | 28 ++++++++++++++++++++++++++--
          1 files changed, 26 insertions, 2 deletions

          Show
          pgee Perry Gee added a comment - This is the code fixes which came out of this review. I will put the results of the review on a wiki page once the PsfFlux bugs are fixed. tickets/ DM-445 in meas_base [pgee@lsst-dev mb5] $ git diff --stat master include/lsst/meas/base/SdssCentroid.h | 3 ++- include/lsst/meas/base/SdssShape.h | 4 ++-- python/lsst/meas/base/plugins.py | 2 +- src/Inputs.cc | 2 +- src/SdssShape.cc | 16 +++++++--------- tests/testClassificationBasic.py | 2 +- 6 files changed, 14 insertions , 15 deletions Add setVersion to the creation of the measurement table: [pgee@lsst-dev pipe_tasks] $ git diff --stat master python/lsst/pipe/tasks/processImage.py | 2 ++ 1 files changed, 2 insertions , 0 deletions Changes to Source.h in u/pgee/s14 [pgee@lsst-dev afw4] $ git diff --stat master include/lsst/afw/table/Source.h.m4 | 28 ++++++++++++++++++++++++++-- 1 files changed, 26 insertions , 2 deletions
          Hide
          jbosch Jim Bosch added a comment -

          I've just pushed two commits on meas_base tickets/DM-445:

          • Fixed the differences between the new and old PsfFlux. See the commit message for more details.
          • I added a new approach to setting the default executionOrder for C+-implemented plugins, so it's no longer necessary to include that in the C+ control objects.

          I'll proceed with the rest of the review tomorrow.

          Show
          jbosch Jim Bosch added a comment - I've just pushed two commits on meas_base tickets/ DM-445 : Fixed the differences between the new and old PsfFlux. See the commit message for more details. I added a new approach to setting the default executionOrder for C+ -implemented plugins, so it's no longer necessary to include that in the C + control objects. I'll proceed with the rest of the review tomorrow.
          Hide
          robyn Robyn Allsman [X] (Inactive) added a comment -

          I'm not sure why you would change the execution order methodology on this
          ticket. Doesn't that belong in another ticket?

          Show
          robyn Robyn Allsman [X] (Inactive) added a comment - I'm not sure why you would change the execution order methodology on this ticket. Doesn't that belong in another ticket?
          Hide
          pgee Perry Gee added a comment -

          I pushed the change to tickets/DM-445. I did not realize it could not be a part of the review in u/pgee/s14. It is the same set of changes, in either case.

          There were a couple of bugfixes included, which hopefully the log reflects. I encountered them during my testing.

          The only change which does not have to be checked in for the code to work is the change to SourceRecord::getCentroid() to return the peak centroid if the result from the centroid slot is nan. I did this to make the Centroid result compatible with the old meas_algorithms, which allowed the centroid to be set even if the centroid algorithm ultimately failed.

          I actually think that we should remove this behavior, and let the Centroid by Nan if the algorithm fails.

          Show
          pgee Perry Gee added a comment - I pushed the change to tickets/ DM-445 . I did not realize it could not be a part of the review in u/pgee/s14. It is the same set of changes, in either case. There were a couple of bugfixes included, which hopefully the log reflects. I encountered them during my testing. The only change which does not have to be checked in for the code to work is the change to SourceRecord::getCentroid() to return the peak centroid if the result from the centroid slot is nan. I did this to make the Centroid result compatible with the old meas_algorithms, which allowed the centroid to be set even if the centroid algorithm ultimately failed. I actually think that we should remove this behavior, and let the Centroid by Nan if the algorithm fails.
          Hide
          jbosch Jim Bosch added a comment - - edited

          I'm not sure why you would change the execution order methodology on this ticket. Doesn't that belong in another ticket?

          I saw that you'd added executionOrder fields to the control objects on this branch, in order to make the centroids happen in the right order. I was planning to bring that up in the review, as I wanted to avoid putting that on master if possible. But I didn't know what to suggest instead, so I experimented a bit, and it turned out the solution was so easy I just went ahead and committed it.

          I pushed the change to tickets/DM-445. I did not realize it could not be a part of the review in u/pgee/s14. It is the same set of changes, in either case.

          Thanks; it's not so much that it couldn't be part of the review on the other branch, it's more that I didn't know if it was intended to be or if it was all just supposed to be temporary

          There were a couple of bugfixes included, which hopefully the log reflects. I encountered them during my testing.
          The only change which does not have to be checked in for the code to work is the change to SourceRecord::getCentroid() to return the peak centroid if the result from the centroid slot is nan. I did this to make the Centroid result compatible with the old meas_algorithms, which allowed the centroid to be set even if the centroid algorithm ultimately failed.

          I actually think that we should remove this behavior, and let the Centroid by Nan if the algorithm fails.

          I agree on all counts. The bugfixes are clearly necessary, and hasCentroid() looks like a useful addition, but I'd like to remove the extra check in getCentroid() before merging to master.

          Show
          jbosch Jim Bosch added a comment - - edited I'm not sure why you would change the execution order methodology on this ticket. Doesn't that belong in another ticket? I saw that you'd added executionOrder fields to the control objects on this branch, in order to make the centroids happen in the right order. I was planning to bring that up in the review, as I wanted to avoid putting that on master if possible. But I didn't know what to suggest instead, so I experimented a bit, and it turned out the solution was so easy I just went ahead and committed it. I pushed the change to tickets/ DM-445 . I did not realize it could not be a part of the review in u/pgee/s14. It is the same set of changes, in either case. Thanks; it's not so much that it couldn't be part of the review on the other branch, it's more that I didn't know if it was intended to be or if it was all just supposed to be temporary There were a couple of bugfixes included, which hopefully the log reflects. I encountered them during my testing. The only change which does not have to be checked in for the code to work is the change to SourceRecord::getCentroid() to return the peak centroid if the result from the centroid slot is nan. I did this to make the Centroid result compatible with the old meas_algorithms, which allowed the centroid to be set even if the centroid algorithm ultimately failed. I actually think that we should remove this behavior, and let the Centroid by Nan if the algorithm fails. I agree on all counts. The bugfixes are clearly necessary, and hasCentroid() looks like a useful addition, but I'd like to remove the extra check in getCentroid() before merging to master.
          Hide
          pgee Perry Gee added a comment -

          This is a test to see to whom this comment gets attributed.

          Show
          pgee Perry Gee added a comment - This is a test to see to whom this comment gets attributed.
          Hide
          jbosch Jim Bosch added a comment -

          Review complete.

          My only major concern is that I'd like to replace the mechanism for choosing table versions in pipe_tasks; I don't think we want to have a separate config option that has to be set by the user to match the version expected by the measurement task. Here's a proposal for an alternative: we add a TABLE_VERSION class attribute constant to the measurement tasks (both the new one and the old one), and have ProcessImageTask do table.setVersion(self.measurement.TABLE_VERSION). That'd require opening up a branch for this issue in meas_algorithms, but I think it's worthwhile.

          I also have two minor issues:

          • I don't think the code to chose the highest numerical flag in SdssShape has any value. Granted, we can't do the right thing on this ticket, but I think this extra logic adds confusion without any additional value. If you choose to leave it, add a comment that it's completely ad-hoc and needs to be replaced.
          • The indentation in Inputs.cc is off, in FootprintCentroidInput's ctor.

          Finally, looking at the concerns you brought up in your email that still need to be resolved, we should update some tickets:

          1. Cannot both set flags and return values in the new framework.

          I see you've already filed this as DM-688.

          2. It seems obvious to me that we should adopt standards for EDGE, NOPIXELS, NOPSF and NOWCS errors so that all algorithms handle these things in the same way.

          Good idea. I could see this being part of DM-461, or I could see it being a new issue; I'll leave which up to you. Could you either add this as a note to DM-461 or open a new issue for it?

          3. More consistency in the configuration options between the fluxes.

          I think you're probably right in general, but I'd like to know what you mean specifically. But please just open a new issue and describe it there.

          Show
          jbosch Jim Bosch added a comment - Review complete. My only major concern is that I'd like to replace the mechanism for choosing table versions in pipe_tasks; I don't think we want to have a separate config option that has to be set by the user to match the version expected by the measurement task. Here's a proposal for an alternative: we add a TABLE_VERSION class attribute constant to the measurement tasks (both the new one and the old one), and have ProcessImageTask do table.setVersion(self.measurement.TABLE_VERSION) . That'd require opening up a branch for this issue in meas_algorithms, but I think it's worthwhile. I also have two minor issues: I don't think the code to chose the highest numerical flag in SdssShape has any value. Granted, we can't do the right thing on this ticket, but I think this extra logic adds confusion without any additional value. If you choose to leave it, add a comment that it's completely ad-hoc and needs to be replaced. The indentation in Inputs.cc is off, in FootprintCentroidInput 's ctor. Finally, looking at the concerns you brought up in your email that still need to be resolved, we should update some tickets: 1. Cannot both set flags and return values in the new framework. I see you've already filed this as DM-688 . 2. It seems obvious to me that we should adopt standards for EDGE, NOPIXELS, NOPSF and NOWCS errors so that all algorithms handle these things in the same way. Good idea. I could see this being part of DM-461 , or I could see it being a new issue; I'll leave which up to you. Could you either add this as a note to DM-461 or open a new issue for it? 3. More consistency in the configuration options between the fluxes. I think you're probably right in general, but I'd like to know what you mean specifically. But please just open a new issue and describe it there.

            People

            Assignee:
            pgee Perry Gee
            Reporter:
            pgee Perry Gee
            Reviewers:
            Jim Bosch
            Watchers:
            Jim Bosch, Perry Gee, Robyn Allsman [X] (Inactive)
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Jenkins

                No builds found.