Refactored to return an optional Table

gtsam/discrete/Signature.cpp

@@ -16,60 +16,57 @@
  * @date Feb 27, 2011
  */
 
-#include "gtsam/discrete/SignatureParser.h"
 #include "Signature.h"
 
-#include <sstream>
 #include <cassert>
+#include <sstream>
+
+#include "gtsam/discrete/SignatureParser.h"
 
 namespace gtsam {
 
-  using namespace std;
+using namespace std;
 
-  ostream& operator <<(ostream &os, const Signature::Row &row) {
+ostream& operator<<(ostream& os, const Signature::Row& row) {
   os << row[0];
-    for (size_t i = 1; i < row.size(); i++)
+  for (size_t i = 1; i < row.size(); i++) os << " " << row[i];
-      os << " " << row[i];
   return os;
-  }
+}
 
-  ostream& operator <<(ostream &os, const Signature::Table &table) {
+ostream& operator<<(ostream& os, const Signature::Table& table) {
-    for (size_t i = 0; i < table.size(); i++)
+  for (size_t i = 0; i < table.size(); i++) os << table[i] << endl;
-      os << table[i] << endl;
   return os;
-  }
+}
 
-  Signature::Signature(const DiscreteKey& key, const DiscreteKeys& parents,
+Signature::Signature(const DiscreteKey& key, const DiscreteKeys& parents,
                      const Table& table)
     : key_(key), parents_(parents) {
   operator=(table);
-  }
+}
 
-  Signature::Signature(const DiscreteKey& key, const DiscreteKeys& parents,
+Signature::Signature(const DiscreteKey& key, const DiscreteKeys& parents,
                      const std::string& spec)
     : key_(key), parents_(parents) {
   operator=(spec);
-  }
+}
 
-  Signature::Signature(const DiscreteKey& key) :
+Signature::Signature(const DiscreteKey& key) : key_(key) {}
-    key_(key) {
-  }
 
-  DiscreteKeys Signature::discreteKeys() const {
+DiscreteKeys Signature::discreteKeys() const {
   DiscreteKeys keys;
   keys.push_back(key_);
   for (const DiscreteKey& key : parents_) keys.push_back(key);
   return keys;
-  }
+}
 
-  KeyVector Signature::indices() const {
+KeyVector Signature::indices() const {
   KeyVector js;
   js.push_back(key_.first);
   for (const DiscreteKey& key : parents_) js.push_back(key.first);
   return js;
-  }
+}
 
-  vector<double> Signature::cpt() const {
+vector<double> Signature::cpt() const {
   vector<double> cpt;
   if (table_) {
     const size_t nrStates = table_->at(0).size();
@@ -81,41 +78,37 @@ namespace gtsam {
     }
   }
   return cpt;
-  }
+}
 
-  Signature& Signature::operator,(const DiscreteKey& parent) {
+Signature &Signature::operator,(const DiscreteKey& parent) {
   parents_.push_back(parent);
   return *this;
-  }
+}
 
-  static void normalize(Signature::Row& row) {
+static void normalize(Signature::Row& row) {
   double sum = 0;
-    for (size_t i = 0; i < row.size(); i++)
+  for (size_t i = 0; i < row.size(); i++) sum += row[i];
-      sum += row[i];
+  for (size_t i = 0; i < row.size(); i++) row[i] /= sum;
-    for (size_t i = 0; i < row.size(); i++)
+}
-      row[i] /= sum;
-  }
 
-  Signature& Signature::operator=(const string& spec) {
+Signature& Signature::operator=(const string& spec) {
   spec_ = spec;
-    Table table;
+  auto table = SignatureParser::Parse(spec);
-    bool success = gtsam::SignatureParser::parse(spec, table);
+  if (table) {
-    if (success) {
+    for (Row& row : *table) normalize(row);
+    table_ = *table;
+  }
+  return *this;
+}
+
+Signature& Signature::operator=(const Table& t) {
+  Table table = t;
   for (Row& row : table) normalize(row);
   table_ = table;
-    }
   return *this;
-  }
+}
 
-  Signature& Signature::operator=(const Table& t) {
+ostream& operator<<(ostream& os, const Signature& s) {
-    Table table = t;
-    for(Row& row: table)
-      normalize(row);
-    table_ = table;
-    return *this;
-  }
-
-  ostream& operator <<(ostream &os, const Signature &s) {
   os << s.key_.first;
   if (s.parents_.empty()) {
     os << " % ";
@@ -131,21 +124,21 @@ namespace gtsam {
   else
     os << "spec could not be parsed" << endl;
   return os;
-  }
+}
 
-  Signature operator|(const DiscreteKey& key, const DiscreteKey& parent) {
+Signature operator|(const DiscreteKey& key, const DiscreteKey& parent) {
   Signature s(key);
   return s, parent;
-  }
+}
 
-  Signature operator%(const DiscreteKey& key, const string& parent) {
+Signature operator%(const DiscreteKey& key, const string& parent) {
   Signature s(key);
   return s = parent;
-  }
+}
 
-  Signature operator%(const DiscreteKey& key, const Signature::Table& parent) {
+Signature operator%(const DiscreteKey& key, const Signature::Table& parent) {
   Signature s(key);
   return s = parent;
-  }
+}
 
 }  // namespace gtsam

gtsam/discrete/SignatureParser.cpp

@@ -2,42 +2,46 @@
 
 #include <algorithm>
 #include <iterator>
+#include <optional>
 #include <sstream>
-
 namespace gtsam {
 
-inline static std::vector<double> ParseTrueRow() { return {0, 1}; }
+using Row = std::vector<double>;
+using Table = std::vector<Row>;
 
-inline static std::vector<double> ParseFalseRow() { return {1, 0}; }
+inline static Row ParseTrueRow() { return {0, 1}; }
 
-inline static SignatureParser::Table ParseOr() {
+inline static Row ParseFalseRow() { return {1, 0}; }
+
+inline static Table ParseOr() {
   return {ParseFalseRow(), ParseTrueRow(), ParseTrueRow(), ParseTrueRow()};
 }
 
-inline static SignatureParser::Table ParseAnd() {
+inline static Table ParseAnd() {
   return {ParseFalseRow(), ParseFalseRow(), ParseFalseRow(), ParseTrueRow()};
 }
 
-bool static ParseConditional(const std::string& token,
+std::optional<Row> static ParseConditional(const std::string& token) {
-                             std::vector<double>& row) {
   // Expect something like a/b/c
   std::istringstream iss2(token);
+  Row row;
   try {
-    // if the string has no / then return false
+    // if the string has no / then return std::nullopt
-    if (std::count(token.begin(), token.end(), '/') == 0) return false;
+    if (std::count(token.begin(), token.end(), '/') == 0) return std::nullopt;
     // split the word on the '/' character
     for (std::string s; std::getline(iss2, s, '/');) {
       // can throw exception
       row.push_back(std::stod(s));
     }
   } catch (...) {
-    return false;
+    return std::nullopt;
   }
-  return true;
+  return row;
 }
 
-void static ParseConditionalTable(const std::vector<std::string>& tokens,
+std::optional<Table> static ParseConditionalTable(
-                                  SignatureParser::Table& table) {
+    const std::vector<std::string>& tokens) {
+  Table table;
   // loop over the words
   // for each word, split it into doubles using a stringstream
   for (const auto& word : tokens) {
@@ -48,14 +52,15 @@ void static ParseConditionalTable(const std::vector<std::string>& tokens,
       table.push_back(ParseTrueRow());
     } else {
       // Expect something like a/b/c
-      std::vector<double> row;
+      if (auto row = ParseConditional(word)) {
-      if (!ParseConditional(word, row)) {
+        table.push_back(*row);
+      } else {
         // stop parsing if we encounter an error
-        return;
+        return std::nullopt;
-      }
-      table.push_back(row);
       }
     }
+  }
+  return table;
 }
 
 std::vector<std::string> static Tokenize(const std::string& str) {
@@ -67,15 +72,15 @@ std::vector<std::string> static Tokenize(const std::string& str) {
   return tokens;
 }
 
-bool SignatureParser::parse(const std::string& str, Table& table) {
+std::optional<Table> SignatureParser::Parse(const std::string& str) {
   // check if string is just whitespace
   if (std::all_of(str.begin(), str.end(), isspace)) {
-    return false;
+    return std::nullopt;
   }
 
-  // return false if the string is empty
+  // return std::nullopt if the string is empty
   if (str.empty()) {
-    return false;
+    return std::nullopt;
   }
 
   // tokenize the str on whitespace
@@ -83,32 +88,30 @@ bool SignatureParser::parse(const std::string& str, Table& table) {
 
   // if the first token is "OR", return the OR table
   if (tokens[0] == "OR") {
-    // if there are more tokens, return false
+    // if there are more tokens, return std::nullopt
     if (tokens.size() > 1) {
-      return false;
+      return std::nullopt;
     }
-    table = ParseOr();
+    return ParseOr();
-    return true;
   }
 
   // if the first token is "AND", return the AND table
   if (tokens[0] == "AND") {
-    // if there are more tokens, return false
+    // if there are more tokens, return std::nullopt
     if (tokens.size() > 1) {
-      return false;
+      return std::nullopt;
     }
-    table = ParseAnd();
+    return ParseAnd();
-    return true;
   }
 
   // otherwise then parse the conditional table
-  ParseConditionalTable(tokens, table);
+  auto table = ParseConditionalTable(tokens);
-  // return false if the table is empty
+  // return std::nullopt if the table is empty
-  if (table.empty()) {
+  if (!table || table->empty()) {
-    return false;
+    return std::nullopt;
   }
   // the boost::phoenix parser did not return an error if we could not fully
   // parse a string it just returned whatever it could parse
-  return true;
+  return table;
 }
 }  // namespace gtsam

gtsam/discrete/SignatureParser.h

@@ -17,7 +17,8 @@
  */
 
 #pragma once
-#include <iostream>
+
+#include <optional>
 #include <string>
 #include <vector>
 
@@ -38,18 +39,18 @@ namespace gtsam {
  *   "1/2/3 2/3/4 1/2/3 2/3/4 1/2/3 2/3/4 1/2/3 2/3/4 1/2/3" :
  *      {{1,2,3},{2,3,4},{1,2,3},{2,3,4},{1,2,3},{2,3,4},{1,2,3},{2,3,4},{1,2,3}}
  *
- * If the string has un-parsable elements, should parse whatever it can:
+ * If the string has un-parsable elements the parser will fail with nullopt:
- *   "1/2 sdf" : {{1,2}}
+ *   "1/2 sdf" : nullopt !
  *
- * It should return false if the string is empty:
+ * It also fails if the string is empty:
- *   "": false
+ *   "": nullopt !
  *
- * We should return false if the rows are not of the same size.
+ * Also fails if the rows are not of the same size.
  */
-namespace SignatureParser {
+struct SignatureParser {
-typedef std::vector<double> Row;
+  using Row = std::vector<double>;
-typedef std::vector<Row> Table;
+  using Table = std::vector<Row>;
 
-bool parse(const std::string& str, Table& table);
+  static std::optional<Table> Parse(const std::string& str);
-};  // namespace SignatureParser
+};
 }  // namespace gtsam

gtsam/discrete/tests/testSignatureParser.cpp

@@ -32,72 +32,62 @@ bool compareTables(const SignatureParser::Table& table1,
 /* ************************************************************************* */
 // Simple test case
 TEST(SimpleParser, Simple) {
-  SignatureParser::Table table, expectedTable;
+  SignatureParser::Table expectedTable{{1, 1}, {2, 3}, {1, 4}};
-  expectedTable = {{1, 1}, {2, 3}, {1, 4}};
+  const auto table = SignatureParser::Parse("1/1 2/3 1/4");
-  bool ret = SignatureParser::parse("1/1 2/3 1/4", table);
+  CHECK(table);
-  EXPECT(ret);
   // compare the tables
-  EXPECT(compareTables(table, expectedTable));
+  EXPECT(compareTables(*table, expectedTable));
 }
 
 /* ************************************************************************* */
 // Test case with each row having 3 elements
 TEST(SimpleParser, ThreeElements) {
-  SignatureParser::Table table, expectedTable;
+  SignatureParser::Table expectedTable{{1, 1, 1}, {2, 3, 2}, {1, 4, 3}};
-  expectedTable = {{1, 1, 1}, {2, 3, 2}, {1, 4, 3}};
+  const auto table = SignatureParser::Parse("1/1/1 2/3/2 1/4/3");
-  bool ret = SignatureParser::parse("1/1/1 2/3/2 1/4/3", table);
+  CHECK(table);
-  EXPECT(ret);
   // compare the tables
-  EXPECT(compareTables(table, expectedTable));
+  EXPECT(compareTables(*table, expectedTable));
 }
 
 /* ************************************************************************* */
 // A test case to check if we can parse a signature with 'T' and 'F'
 TEST(SimpleParser, TAndF) {
-  SignatureParser::Table table, expectedTable;
+  SignatureParser::Table expectedTable{{1, 0}, {1, 0}, {1, 0}, {0, 1}};
-  expectedTable = {{1, 0}, {1, 0}, {1, 0}, {0, 1}};
+  const auto table = SignatureParser::Parse("F F F T");
-  bool ret = SignatureParser::parse("F F F T", table);
+  CHECK(table);
-  EXPECT(ret);
   // compare the tables
-  EXPECT(compareTables(table, expectedTable));
+  EXPECT(compareTables(*table, expectedTable));
 }
 
 /* ************************************************************************* */
 // A test to parse {F F F 1}
 TEST(SimpleParser, FFF1) {
-  SignatureParser::Table table, expectedTable;
+  SignatureParser::Table expectedTable{{1, 0}, {1, 0}, {1, 0}};
-  expectedTable = {{1, 0}, {1, 0}, {1, 0}};
+  const auto table = SignatureParser::Parse("F F F");
-  // should ignore the last 1
+  CHECK(table);
-  bool ret = SignatureParser::parse("F F F 1", table);
-  EXPECT(ret);
   // compare the tables
-  EXPECT(compareTables(table, expectedTable));
+  EXPECT(compareTables(*table, expectedTable));
 }
 
 /* ************************************************************************* */
 // Expect false if the string is empty
 TEST(SimpleParser, emptyString) {
-  SignatureParser::Table table;
+  const auto table = SignatureParser::Parse("");
-  bool ret = SignatureParser::parse("", table);
+  EXPECT(!table);
-  EXPECT(!ret);
 }
 
 /* ************************************************************************* */
 // Expect false if gibberish
 TEST(SimpleParser, Gibberish) {
-  SignatureParser::Table table;
+  const auto table = SignatureParser::Parse("sdf 22/3");
-  bool ret = SignatureParser::parse("sdf 22/3", table);
+  EXPECT(!table);
-  EXPECT(!ret);
 }
 
-// If Gibberish is in the middle, it should still parse the rest
+// If Gibberish is in the middle, it should not parse.
 TEST(SimpleParser, GibberishInMiddle) {
-  SignatureParser::Table table, expectedTable;
+  SignatureParser::Table expectedTable{{1, 1}, {2, 3}};
-  expectedTable = {{1, 1}, {2, 3}};
+  const auto table = SignatureParser::Parse("1/1 2/3 sdf 1/4");
-  bool ret = SignatureParser::parse("1/1 2/3 sdf 1/4", table);
+  EXPECT(!table);
-  EXPECT(ret);
-  // compare the tables
-  EXPECT(compareTables(table, expectedTable));
 }
 
 /* ************************************************************************* */