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

Documentation audit and cleanup for meas_base plugins

    XMLWordPrintable

    Details

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

      Description

      Many meas_base Plugins and Algorithms have poor documentation, including several whose documentation is a copy/paste relic from some other algorithm. These need to be fixed.

        Attachments

          Issue Links

            Activity

            Hide
            jbosch Jim Bosch added a comment -

            Perry started work on this, and I'll take it to completion while he's on vacation.

            Show
            jbosch Jim Bosch added a comment - Perry started work on this, and I'll take it to completion while he's on vacation.
            Hide
            jbosch Jim Bosch added a comment -

            Ready for review. Dick, could you take a look at this? It's almost entirely documentation, with a few small fixes thrown in (mostly copy/paste relics from creating a new algorithm by duplicating an old one), and I expect you'll find it useful reading.

            You can see all the changes on this branch by repeating the command below without the --stat option (all changes are in meas_base):

            meas_base:tickets/DM-977 % git diff --stat origin/master origin/tickets/DM-977
             include/lsst/meas/base/GaussianCentroid.h |   27 +++++++-----------
             include/lsst/meas/base/GaussianFlux.h     |   30 ++++++--------------
             include/lsst/meas/base/NaiveCentroid.h    |   27 ++++++------------
             include/lsst/meas/base/NaiveFlux.h        |   26 ++++++++---------
             include/lsst/meas/base/PixelFlags.h       |   11 ++++----
             include/lsst/meas/base/PsfFlux.h          |    5 ++--
             include/lsst/meas/base/SdssCentroid.h     |   14 ++++------
             include/lsst/meas/base/SincFlux.h         |   21 ++++++++------
             python/lsst/meas/base/plugins.py          |   43 +++++++++++++++++------------
             src/GaussianCentroid.cc                   |    4 ---
             src/NaiveCentroid.cc                      |    3 --
             src/SdssCentroid.cc                       |    3 --
             src/SincFlux.cc                           |    9 ------
             tests/testClassificationBasic.py          |    9 +++---
             tests/testGaussianCentroidBasic.py        |    1 -
             tests/testSincFluxBasic.py                |    3 --
             16 files changed, 93 insertions(+), 143 deletions(-)

            Note that I've opened a couple of other issues (DM-1014, DM-1016) to get more documentation from Robert Lupton on a few of these algorithms, and the documentation for the Task code is also a separate issue (DM-076). And there are many, many separate issues for code cleanup.

            Show
            jbosch Jim Bosch added a comment - Ready for review. Dick, could you take a look at this? It's almost entirely documentation, with a few small fixes thrown in (mostly copy/paste relics from creating a new algorithm by duplicating an old one), and I expect you'll find it useful reading. You can see all the changes on this branch by repeating the command below without the --stat option (all changes are in meas_base): meas_base:tickets/DM-977 % git diff --stat origin/master origin/tickets/DM-977 include/lsst/meas/base/GaussianCentroid.h | 27 +++++++----------- include/lsst/meas/base/GaussianFlux.h | 30 ++++++-------------- include/lsst/meas/base/NaiveCentroid.h | 27 ++++++------------ include/lsst/meas/base/NaiveFlux.h | 26 ++++++++--------- include/lsst/meas/base/PixelFlags.h | 11 ++++---- include/lsst/meas/base/PsfFlux.h | 5 ++-- include/lsst/meas/base/SdssCentroid.h | 14 ++++------ include/lsst/meas/base/SincFlux.h | 21 ++++++++------ python/lsst/meas/base/plugins.py | 43 +++++++++++++++++------------ src/GaussianCentroid.cc | 4 --- src/NaiveCentroid.cc | 3 -- src/SdssCentroid.cc | 3 -- src/SincFlux.cc | 9 ------ tests/testClassificationBasic.py | 9 +++--- tests/testGaussianCentroidBasic.py | 1 - tests/testSincFluxBasic.py | 3 -- 16 files changed, 93 insertions(+), 143 deletions(-) Note that I've opened a couple of other issues ( DM-1014 , DM-1016 ) to get more documentation from Robert Lupton on a few of these algorithms, and the documentation for the Task code is also a separate issue ( DM-076 ). And there are many, many separate issues for code cleanup.
            Hide
            shaw Richard Shaw [X] (Inactive) added a comment -

            Ok Jim, I'll have a look.

            Show
            shaw Richard Shaw [X] (Inactive) added a comment - Ok Jim, I'll have a look.
            Hide
            shaw Richard Shaw [X] (Inactive) added a comment -

            The changes are mostly additions, corrections, or clarifications to the comments that are improvements (sometimes substantial) over the previous doc strings. This information is indeed useful material for crafting higher-level docs. The need for additional discussion on the functionality of some algorithms is noted.

            I cannot comment on the changes to the source code. For the doc strings, there are a few points to consider:

            1. GaussianCentroidAlgorithm: the reference to getFlagDefinitions() is a very handy piece of information, though perhaps it would be helpful to mention its location (PixelFlags.h). It seems this bit of code deserves a much higher profile in the documentation. I can refer to it in the Confluence docs, but is it referenced elsewhere in the task-level docs?

            2. PixelFlagsAlgorithm::Result: my parser gagged on this comment:

            • ResultMapper for PixelFlagsAlgorithm, which only defines a flag set out its output.

            3. This may be off-topic for this review, but where would one find an explanation of the plug-in framework? This would be helpful reading for someone trying to understand the code and comments. (I assume it exists, but I don't know where.)

            Show
            shaw Richard Shaw [X] (Inactive) added a comment - The changes are mostly additions, corrections, or clarifications to the comments that are improvements (sometimes substantial) over the previous doc strings. This information is indeed useful material for crafting higher-level docs. The need for additional discussion on the functionality of some algorithms is noted. I cannot comment on the changes to the source code. For the doc strings, there are a few points to consider: 1. GaussianCentroidAlgorithm: the reference to getFlagDefinitions() is a very handy piece of information, though perhaps it would be helpful to mention its location (PixelFlags.h). It seems this bit of code deserves a much higher profile in the documentation. I can refer to it in the Confluence docs, but is it referenced elsewhere in the task-level docs? 2. PixelFlagsAlgorithm::Result: my parser gagged on this comment: ResultMapper for PixelFlagsAlgorithm, which only defines a flag set out its output. 3. This may be off-topic for this review, but where would one find an explanation of the plug-in framework? This would be helpful reading for someone trying to understand the code and comments. (I assume it exists, but I don't know where.)
            Hide
            jbosch Jim Bosch added a comment -

            1. GaussianCentroidAlgorithm: the reference to getFlagDefinitions() is a very handy piece of information, though perhaps it would be helpful to mention its location (PixelFlags.h). It seems this bit of code deserves a much higher profile in the documentation. I can refer to it in the Confluence docs, but is it referenced elsewhere in the task-level docs?

            Actually, this was referring to the getFlagDefinitions() in GaussianCentroidAlgorithm, not the one in PixelFlags. Each algorithm implemented in C++ has its own flags, and PixelFlags just has a larger set because it's an algorithm that only sets flags (PixelFlags is a measurement algorithms that simply looks for which mask bits are set within the Footprint of a source).

            This isn't mentioned in the Task docs yet, but that's still a work in progress (DM-976).

            2. PixelFlagsAlgorithm::Result: my parser gagged on this comment:

            • ResultMapper for PixelFlagsAlgorithm, which only defines a flag set out its output.

            Fixed (to "which includes only flag fields").

            3. This may be off-topic for this review, but where would one find an explanation of the plug-in framework? This would be helpful reading for someone trying to understand the code and comments. (I assume it exists, but I don't know where.)

            There's some in the Doxygen package overview page (mostly from the perspective of how to write new plugins). There'll be more coming in DM-976 (and probably some future tickets).

            Show
            jbosch Jim Bosch added a comment - 1. GaussianCentroidAlgorithm: the reference to getFlagDefinitions() is a very handy piece of information, though perhaps it would be helpful to mention its location (PixelFlags.h). It seems this bit of code deserves a much higher profile in the documentation. I can refer to it in the Confluence docs, but is it referenced elsewhere in the task-level docs? Actually, this was referring to the getFlagDefinitions() in GaussianCentroidAlgorithm , not the one in PixelFlags . Each algorithm implemented in C++ has its own flags, and PixelFlags just has a larger set because it's an algorithm that only sets flags ( PixelFlags is a measurement algorithms that simply looks for which mask bits are set within the Footprint of a source). This isn't mentioned in the Task docs yet, but that's still a work in progress ( DM-976 ). 2. PixelFlagsAlgorithm::Result: my parser gagged on this comment: ResultMapper for PixelFlagsAlgorithm, which only defines a flag set out its output. Fixed (to "which includes only flag fields"). 3. This may be off-topic for this review, but where would one find an explanation of the plug-in framework? This would be helpful reading for someone trying to understand the code and comments. (I assume it exists, but I don't know where.) There's some in the Doxygen package overview page (mostly from the perspective of how to write new plugins). There'll be more coming in DM-976 (and probably some future tickets).

              People

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

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.