# Documentation audit and cleanup for meas_base plugins

XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• Story Points:
2
• Sprint:
Measurement-S14-3
• Team:
Data Release Production

#### 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.

#### Activity

Hide
Jim Bosch added a comment -

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

Show
Jim Bosch added a comment - Perry started work on this, and I'll take it to completion while he's on vacation.
Hide
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
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
Richard Shaw [X] (Inactive) added a comment -

Ok Jim, I'll have a look.

Show
Richard Shaw [X] (Inactive) added a comment - Ok Jim, I'll have a look.
Hide
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
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
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
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:
Jim Bosch
Reporter:
Jim Bosch
Reviewers:
Richard Shaw [X] (Inactive)
Watchers:
Jim Bosch, Perry Gee, Richard Shaw [X] (Inactive)