DM-9249 Change FlagHandler to remove the enum of flag types #68

pgee2000 · 2017-02-27T06:56:26Z

enum is replaced with static FlagDefinition of the same name
If the index of the flag is required a FlagDefinition.number
If the flag name is required, use FlagDefinition.name

Remove the old style of initializing the FlagHandler using iterator
to a std::array. A new class FlagDefinitionList
is introduced to specify the FlagDefinitions used to initialize the
FlagHandler. Add flags to the FlagDefinitionList using the methods
addFailureFlag() and add(name, doc). When done, use the FlagHandler
initializer addFields(schema, name, flagDefinitionList).

An optional exclusion list exclDefs is provided if some members of
the algorithms FlagDefinitionList are not desired for a particular
configuration. The _flagHandler keeps the same numbering of flags
whether or not a flag is excluded -- the FlagDefinition.number is
fixed. But the flag is not added to the schema if it is excluded.

It is now optional whether a General Failure flag is defined for the
flagHandler. If it is not added to the FlagDefinitionList, the
flagHandler will not perform the function of setting the general
flag when the other flags are set in handleFailure.

Remove the FlagHandler::FAILURE which was used to always identify
the failure flag as number 0. Flags may be added in any order.
Use flagHandler.getFailureFlagNumber() to get the correct index.

rearmstr · 2017-03-02T15:34:31Z

I would clean up the wording in the first paragraph of the commit. It was a little hard to parse.

There is a lot of information in this commit message that might be a better fit in documentation. Perhaps, you give a more concise summary of the work that was done.

rearmstr · 2017-03-02T15:36:26Z

include/lsst/meas/base/ApertureFlux.h

+    bool getFlag(unsigned int index) const { return _flags[index]; }
+
+    /// Return the flag value associated with the given flag name
+    bool getFlag(std::string name) const { return _flags[ApertureFluxAlgorithm::getFlagDefinitions().getDefinition(name).number]; }


This line is too long. There are also many other lines below that are beyond the 110 char limit.

rearmstr · 2017-03-02T15:42:03Z

include/lsst/meas/base/FlagHandler.h

+     *  @brief Add a new FlagDefinition to this list. Return a copy with the
+     *  FlagDefinition.number set corresponding to its index in the list.
+     */
+    FlagDefinition add(std::string const name, std::string const doc) {


Should these arguments be changed to const ref's?

Fix. Sorry, my comment on Jira doesn't apply. I misread this review comment

rearmstr · 2017-03-02T15:44:16Z

include/lsst/meas/base/FlagHandler.h


    /**
     *  Default constructor for delayed initialization.
     *
-     *  This constructor creates an invalid, unusable FlagHandler in the same way an iterator
-     *  default constructor constructs an invalid iterator.  Its only purpose is to delay construction
+     *  This constructor creates an invalid, unusable FlagHandler in the same way an const_iterator


"an const_iterator" -> "a const_iterator"

rearmstr · 2017-03-02T15:50:13Z

include/lsst/meas/base/FlagHandler.h

 private:
-


removal of blank lines should be put in a separate commit. There are a couple of other examples of this below.

rearmstr · 2017-03-02T16:08:38Z

src/GaussianCentroid.cc

+    for (std::size_t i = 0; i < GaussianCentroidAlgorithm::getFlagDefinitions().size(); i++) {
+        FlagDefinition const & flag = GaussianCentroidAlgorithm::getFlagDefinitions()[i];
+        if (flag == GaussianCentroidAlgorithm::FAILURE) continue;
+        if (mapper.getInputSchema().getNames().count(name + "_" + flag.name) == 0) continue;


Shouldn't you be using the schema.join method to look for the name? Same comment for other transform algorithms below.

rearmstr · 2017-03-02T16:11:37Z

src/GaussianCentroid.cc

-                          mapper.getInputSchema().join(name, flag->name)).key);
+    for (std::size_t i = 0; i < GaussianCentroidAlgorithm::getFlagDefinitions().size(); i++) {
+        FlagDefinition const & flag = GaussianCentroidAlgorithm::getFlagDefinitions()[i];
+        if (flag == GaussianCentroidAlgorithm::FAILURE) continue;


Why does this not get added to the mapper? Same comment for other transform algorithms below.

I copied what was in the existing code.

rearmstr · 2017-03-02T16:42:29Z

One general comment. I'm not crazy about the need to specify ".number" when setting flags in a record. Maybe this is the price to pay for being consistent everywhere and allowing for some flags to not be added to schemas.

rearmstr · 2017-03-02T16:44:31Z

src/NaiveCentroid.cc

 }

+
+namespace {


Not sure why you have an empty namespace here? There is another example of this below.

rearmstr · 2017-03-02T16:51:52Z

src/SdssShape.cc

@@ -734,7 +743,8 @@ SdssShapeResult SdssShapeResultKey::get(afw::table::BaseRecord const & record) c
    result.flux_xx_Cov = record.get(_flux_xx_Cov);
    result.flux_yy_Cov = record.get(_flux_yy_Cov);
    result.flux_xy_Cov = record.get(_flux_xy_Cov);
-    for (int n = 0; n < SdssShapeAlgorithm::N_FLAGS - (_includePsf ? 0 : 1); ++n) {
+    for (unsigned int n = 0; n < SdssShapeAlgorithm::N_FLAGS; ++n) {


Here and below you are using unsigned int. The rest of the code in these pull requests are using size_t, it would be good to be consistent.

The enum which formerly numbered the available failure flags is replaced with a set of static FlagDefinitions: (e.g., static FlagDefinition const FAILURE; for the general flag). The position of the flag in the FlagHandler list is now the number value of the FlagDefinition (e.g., FAILURE.number). A new class FlagDefinitionList is introduced to specify the FlagDefinitions used to initialize the FlagHandler. Add flags to the FlagDefinitionList using the methods addFailureFlag() and add(name, doc). When done, use the FlagHandler initializer addFields(schema, name, flagDefinitionList). An optional exclusion list exclDefs is provided if some members of the algorithms FlagDefinitionList are not desired. The _flagHandler keeps the same numbering of flags whether or not a flag is excluded -- the FlagDefinition.number is fixed. It is now optional whether a General Failure flag is defined for the flagHandler. Remove the FlagHandler::FAILURE which assumes FAILURE is position 0.

rearmstr reviewed Mar 2, 2017

View reviewed changes

pgee2000 force-pushed the tickets/DM-9249 branch from a17aaed to b10abc3 Compare March 3, 2017 17:24

pgee2000 force-pushed the tickets/DM-9249 branch from b10abc3 to 4c2df6d Compare March 3, 2017 19:37

pgee2000 merged commit 4c2df6d into master Mar 3, 2017

ktlim deleted the tickets/DM-9249 branch August 25, 2018 06:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-9249 Change FlagHandler to remove the enum of flag types #68

DM-9249 Change FlagHandler to remove the enum of flag types #68

pgee2000 commented Feb 27, 2017

rearmstr commented Mar 2, 2017

rearmstr Mar 2, 2017 •

edited

pgee2000 Mar 3, 2017

rearmstr Mar 2, 2017

pgee2000 Mar 3, 2017

rearmstr Mar 2, 2017

pgee2000 Mar 3, 2017

rearmstr Mar 2, 2017

pgee2000 Mar 3, 2017

rearmstr Mar 2, 2017 •

edited

pgee2000 Mar 3, 2017

rearmstr Mar 2, 2017 •

edited

pgee2000 Mar 3, 2017

rearmstr commented Mar 2, 2017

rearmstr Mar 2, 2017

pgee2000 Mar 3, 2017

rearmstr Mar 2, 2017

pgee2000 Mar 3, 2017

DM-9249 Change FlagHandler to remove the enum of flag types #68

DM-9249 Change FlagHandler to remove the enum of flag types #68

Conversation

pgee2000 commented Feb 27, 2017

rearmstr commented Mar 2, 2017

rearmstr Mar 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rearmstr Mar 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rearmstr Mar 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rearmstr commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rearmstr Mar 2, 2017 •

edited

rearmstr Mar 2, 2017 •

edited

rearmstr Mar 2, 2017 •

edited