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

Setup of four new measurement algorithms for processCcd testing

    XMLWordPrintable

    Details

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

      Description

      The goal of this story is to take the following algorithms and make them fully operational with processCcd.

      PsfFlux,SdssShape,SdssCentroid,SincFlux.

      This code was moved to meas_base in w14, but has yet to be used in full operation. This is the first step in that process.

      Each algorithm will at least have one test to confirm that it works, but the unit tests will be very simple. The actual confirmation of full operability will the to run tests against real data, and compare against the existing meas_algorithms.

      The algorithm code will still require cleanup even after this story is completed. That is because it has intentionally been left the same as what existed in meas_algorithms.

      The goal of this ticket is to confirm that the each algorithm can (1) complete its measurement with some reasonable result, (2) responds to its major configuration options, and (3) handles at least some of its defined exceptions (that is, flags) correctly.

      Confirmation that the results are identical with meas_algorithms is a subsequent ticket

      Cleanup and documentation of this code will be done in a subsequent ticket.

        Attachments

          Activity

          Hide
          jbosch Jim Bosch added a comment -

          I see changes on tickets/DM-441 of meas_base, but it seems like there must be changes in meas_algorithms as well (because SdssCentroid.cc includes a file from meas_algorithms that doesn't exist on master). Do you need to push a branch for that as well?

          Show
          jbosch Jim Bosch added a comment - I see changes on tickets/ DM-441 of meas_base, but it seems like there must be changes in meas_algorithms as well (because SdssCentroid.cc includes a file from meas_algorithms that doesn't exist on master). Do you need to push a branch for that as well?
          Hide
          robyn Robyn Allsman [X] (Inactive) added a comment -

          I moved all the code from meas_algorithms to meas/base/algorithms.

          git is not saying that I haven't pushed it. Are you not seeing it?

          Show
          robyn Robyn Allsman [X] (Inactive) added a comment - I moved all the code from meas_algorithms to meas/base/algorithms. git is not saying that I haven't pushed it. Are you not seeing it?
          Hide
          jbosch Jim Bosch added a comment -

          Ah, I did misread that include line that I thought pointed at meas_algorithms, but you're right, I'm not seeing that directory at all. Sometimes git is a little weird about directories; try doing a "git add" on the algorithms directories or files within them.

          Also, even weirder: I'm 100% that last comment on this issue was from Perry, but JIRA seems to think it was Robyn (both in the notification email l got and on the page I see now); what does it look like to you?

          Show
          jbosch Jim Bosch added a comment - Ah, I did misread that include line that I thought pointed at meas_algorithms, but you're right, I'm not seeing that directory at all. Sometimes git is a little weird about directories; try doing a "git add" on the algorithms directories or files within them. Also, even weirder: I'm 100% that last comment on this issue was from Perry, but JIRA seems to think it was Robyn (both in the notification email l got and on the page I see now); what does it look like to you?
          Hide
          pgee Perry Gee added a comment -

          Yes, it never the stuff from meas_algorithms checked in. But git did not tell me about that until I added algorithms to both include and src.

          Don't forget that they are in a raw form, until the later cleanup ticket. The placement of code in include and src is really weird, in my opinion. But I think the big test at this point is compatibility with the old framework.

          One issue is that I may have had to change to namespace from anonymous to meas::base::algorithms for visibility. Don't know if that could have any side effects. So you might look at that.

          Show
          pgee Perry Gee added a comment - Yes, it never the stuff from meas_algorithms checked in. But git did not tell me about that until I added algorithms to both include and src. Don't forget that they are in a raw form, until the later cleanup ticket. The placement of code in include and src is really weird, in my opinion. But I think the big test at this point is compatibility with the old framework. One issue is that I may have had to change to namespace from anonymous to meas::base::algorithms for visibility. Don't know if that could have any side effects. So you might look at that.
          Hide
          jbosch Jim Bosch added a comment -

          Please let me know when you've added SkyCoord to this branch.

          Show
          jbosch Jim Bosch added a comment - Please let me know when you've added SkyCoord to this branch.
          Hide
          pgee Perry Gee added a comment -

          I checked in what I have. I don't think the infrastructure is right for doing the error handling for skycoord, but it should work otherwise. I will add the Python error handling issue to one of the later tickets.

          Show
          pgee Perry Gee added a comment - I checked in what I have. I don't think the infrastructure is right for doing the error handling for skycoord, but it should work otherwise. I will add the Python error handling issue to one of the later tickets.
          Hide
          jbosch Jim Bosch added a comment -

          Looks good! All of my comments are minor and mostly stylistic, so hopefully they won't be too hard
          to do on this ticket, but feel free to push them to future issues if they're hard. I know we've got
          a lot of dependent issues tangled up together right now, and I'm sure you're eager to get this on
          master.

          General:

          • I have not looked at all at the stuff in the "algorithms"
            directories at all, as we've already decided that cleaning up that
            code will go on a future issue, and I don't want to burden you with my
            cleanup ideas before you've had a chance to do yours.
          • The move from anonymous namespaces to meas::base::algorithms shouldn't cause any problems outside
            of meas_base, so if it's working there (and it appears to be), it's not something we need to
            worry about.
          • I'm not sure we should copy the Doxygen comments from SdssShape and PsfFlux as to the others as
            completely as you have; a lot of those docs are about serving as an example for someone implementing
            a new algorithm, and while that's fine for 1 or 2 algorithms, for most algorithms the docs should
            talk about what that algorithm actually does (or just reference the base class docs, if it's purely
            the implementation of an interface). There's a lot of duplication in the docs right now.
          • It looks like those Doxygen @cond/@endcond blocks in the Source files (something you copied from
            me, I think) don't actually work. We don't actually appear to have any way to get Doxygen to run
            on these files and produce reasonable results. Not sure if it's worth fixing now, but we shouldn't
            copy that failed technique to future files.

          Inputs.cc:

          • <iostream> include seems unnecessary.
          • When checking whether a slot is defined, as in the FootprintCentroidInput constructor, I think it's better to use table.getCentroidKey().isValid() rather than getCentroidDefinition().size().

          Results.h:

          • FloatComponent is commented out; did you intend to remove it?

          plugins.py:

          • If PeakCentroid doesn't set the xSigma and ySigma fields, it shouldn't allocate them in the constructor. Does something not work if they're missing?

          all.h:

          • Was this file supposed to be committed? It seems pretty irregular, and I don't see it being used anywhere.

          SdssCentroid.h:

          • All the standard library includes here should probably go in the .cc file, as I don't think they're actually needed in the header. And if we do need these, they should probably be using e.g. "#include <cstdio>" instead of "#include <stdio.h>" (see http://www.parashift.com/c++-faq/include-c-hdrs-system.html).

          SdssCentroid.cc:

          • ndarray headers should go after the standard library headers; the order should be system headers, third-party headers, LSST headers.

          SdssShape.cc:

          • I understand you have some more work you want to do here (I see you have some TODOs). Could you just
            clarify on the issue page whether you want to do that on this issue or on a future one?
          Show
          jbosch Jim Bosch added a comment - Looks good! All of my comments are minor and mostly stylistic, so hopefully they won't be too hard to do on this ticket, but feel free to push them to future issues if they're hard. I know we've got a lot of dependent issues tangled up together right now, and I'm sure you're eager to get this on master. General: I have not looked at all at the stuff in the "algorithms" directories at all, as we've already decided that cleaning up that code will go on a future issue, and I don't want to burden you with my cleanup ideas before you've had a chance to do yours. The move from anonymous namespaces to meas::base::algorithms shouldn't cause any problems outside of meas_base, so if it's working there (and it appears to be), it's not something we need to worry about. I'm not sure we should copy the Doxygen comments from SdssShape and PsfFlux as to the others as completely as you have; a lot of those docs are about serving as an example for someone implementing a new algorithm, and while that's fine for 1 or 2 algorithms, for most algorithms the docs should talk about what that algorithm actually does (or just reference the base class docs, if it's purely the implementation of an interface). There's a lot of duplication in the docs right now. It looks like those Doxygen @cond/@endcond blocks in the Source files (something you copied from me, I think) don't actually work. We don't actually appear to have any way to get Doxygen to run on these files and produce reasonable results. Not sure if it's worth fixing now, but we shouldn't copy that failed technique to future files. Inputs.cc: <iostream> include seems unnecessary. When checking whether a slot is defined, as in the FootprintCentroidInput constructor, I think it's better to use table.getCentroidKey().isValid() rather than getCentroidDefinition().size(). Results.h: FloatComponent is commented out; did you intend to remove it? plugins.py: If PeakCentroid doesn't set the xSigma and ySigma fields, it shouldn't allocate them in the constructor. Does something not work if they're missing? all.h: Was this file supposed to be committed? It seems pretty irregular, and I don't see it being used anywhere. SdssCentroid.h: All the standard library includes here should probably go in the .cc file, as I don't think they're actually needed in the header. And if we do need these, they should probably be using e.g. "#include <cstdio>" instead of "#include <stdio.h>" (see http://www.parashift.com/c++-faq/include-c-hdrs-system.html ). SdssCentroid.cc: ndarray headers should go after the standard library headers; the order should be system headers, third-party headers, LSST headers. SdssShape.cc: I understand you have some more work you want to do here (I see you have some TODOs). Could you just clarify on the issue page whether you want to do that on this issue or on a future one?
          Hide
          pgee Perry Gee added a comment -

          all.h was used by one of the algorithms. I will leave it out until it is needed.

          I am going to finish SdssShape before I check in

          Show
          pgee Perry Gee added a comment - all.h was used by one of the algorithms. I will leave it out until it is needed. I am going to finish SdssShape before I check in

            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.