From 8288b55d4e10181e3c2eaf2328815a69167b5671 Mon Sep 17 00:00:00 2001
From: Frank Dellaert <dellaert@gmail.com>
Date: Sun, 16 Aug 2020 17:30:52 -0400
Subject: [PATCH] Addressed review comments

---
 gtsam/slam/dataset.cpp         | 30 ++++++++++++++++++++----------
 gtsam/slam/dataset.h           |  4 ++--
 timing/timeFrobeniusFactor.cpp |  4 ++--
 3 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/gtsam/slam/dataset.cpp b/gtsam/slam/dataset.cpp
index 12147adf2..d7b067d70 100644
--- a/gtsam/slam/dataset.cpp
+++ b/gtsam/slam/dataset.cpp
@@ -110,19 +110,21 @@ string createRewrittenFileName(const string &name) {
 }
 
 /* ************************************************************************* */
-// Parse a file by calling the parse(is, tag) function for every line.
+// Type for parser functions used in parseLines below.
 template <typename T>
 using Parser =
     std::function<boost::optional<T>(istream &is, const string &tag)>;
 
+// Parse a file by calling the parse(is, tag) function for every line.
+// The result of calling the function is ignored, so typically parse function
+// works via a side effect, like adding a factor into a graph etc.
 template <typename T>
 static void parseLines(const string &filename, Parser<T> parse) {
   ifstream is(filename.c_str());
   if (!is)
     throw invalid_argument("parse: can not find file " + filename);
-  while (!is.eof()) {
-    string tag;
-    is >> tag;
+  string tag;
+  while (is >> tag) {
     parse(is, tag); // ignore return value
     is.ignore(LINESIZE, '\n');
   }
@@ -164,7 +166,9 @@ boost::optional<IndexedPose> parseVertexPose(istream &is, const string &tag) {
   if ((tag == "VERTEX2") || (tag == "VERTEX_SE2") || (tag == "VERTEX")) {
     size_t id;
     double x, y, yaw;
-    is >> id >> x >> y >> yaw;
+    if (!(is >> id >> x >> y >> yaw)) {
+      throw std::runtime_error("parseVertexPose encountered malformed line");
+    }
     return IndexedPose(id, Pose2(x, y, yaw));
   } else {
     return boost::none;
@@ -183,7 +187,10 @@ boost::optional<IndexedLandmark> parseVertexLandmark(istream &is,
   if (tag == "VERTEX_XY") {
     size_t id;
     double x, y;
-    is >> id >> x >> y;
+    if (!(is >> id >> x >> y)) {
+      throw std::runtime_error(
+          "parseVertexLandmark encountered malformed line");
+    }
     return IndexedLandmark(id, Point2(x, y));
   } else {
     return boost::none;
@@ -287,7 +294,9 @@ boost::optional<IndexedEdge> parseEdge(istream &is, const string &tag) {
 
     size_t id1, id2;
     double x, y, yaw;
-    is >> id1 >> id2 >> x >> y >> yaw;
+    if (!(is >> id1 >> id2 >> x >> y >> yaw)) {
+      throw std::runtime_error("parseEdge encountered malformed line");
+    }
     return IndexedEdge({id1, id2}, Pose2(x, y, yaw));
   } else {
     return boost::none;
@@ -295,8 +304,8 @@ boost::optional<IndexedEdge> parseEdge(istream &is, const string &tag) {
 }
 
 /* ************************************************************************* */
-// Measurement parsers are implemented as a functor, as they has several options
-// to filter and create the measurement model.
+// Measurement parsers are implemented as a functor, as they have several
+// options to filter and create the measurement model.
 template <typename T> struct ParseMeasurement;
 
 /* ************************************************************************* */
@@ -670,7 +679,8 @@ void writeG2o(const NonlinearFactorGraph &graph, const Values &estimate,
     auto factor = boost::dynamic_pointer_cast<BetweenFactor<Pose2>>(factor_);
     if (factor) {
       SharedNoiseModel model = factor->noiseModel();
-      auto gaussianModel = boost::dynamic_pointer_cast<noiseModel::Gaussian>(model);
+      auto gaussianModel =
+          boost::dynamic_pointer_cast<noiseModel::Gaussian>(model);
       if (!gaussianModel) {
         model->print("model\n");
         throw invalid_argument("writeG2o: invalid noise model!");
diff --git a/gtsam/slam/dataset.h b/gtsam/slam/dataset.h
index 7bdf6728d..53abe55ba 100644
--- a/gtsam/slam/dataset.h
+++ b/gtsam/slam/dataset.h
@@ -77,7 +77,7 @@ enum KernelFunctionType {
  * Parse variables in a line-based text format (like g2o) into a map.
  * Instantiated in .cpp Pose2, Point2, Pose3, and Point3.
  * Note the map keys are integer indices, *not* gtsam::Keys. This is is
- * different below where landmarks will use be L(index) symbols.
+ * different below where landmarks will use L(index) symbols.
  */
 template <typename T>
 GTSAM_EXPORT std::map<size_t, T> parseVariables(const std::string &filename,
@@ -136,7 +136,7 @@ GTSAM_EXPORT boost::optional<IndexedEdge> parseEdge(std::istream& is,
 
 /// Return type for load functions, which return a graph and initial values. For
 /// landmarks, the gtsam::Symbol L(index) is used to insert into the Values.
-/// Bearing-range measurements also refer tio landmarks with L(index).
+/// Bearing-range measurements also refer to landmarks with L(index).
 using GraphAndValues =
     std::pair<NonlinearFactorGraph::shared_ptr, Values::shared_ptr>;
 
diff --git a/timing/timeFrobeniusFactor.cpp b/timing/timeFrobeniusFactor.cpp
index c0ac28ed9..924213a33 100644
--- a/timing/timeFrobeniusFactor.cpp
+++ b/timing/timeFrobeniusFactor.cpp
@@ -68,10 +68,10 @@ int main(int argc, char* argv[]) {
   auto G = boost::make_shared<Matrix>(SOn::VectorizedGenerators(4));
   for (const auto &m : measurements) {
     const auto &keys = m.keys();
-    const Rot3 &Tij = m.measured();
+    const Rot3 &Rij = m.measured();
     const auto &model = m.noiseModel();
     graph.emplace_shared<FrobeniusWormholeFactor>(
-        keys[0], keys[1], Tij, 4, model, G);
+        keys[0], keys[1], Rij, 4, model, G);
   }
 
   std::mt19937 rng(42);