3387 - Add option to diff conflate to keep reviews in output and more

conf/core/ConfigOptions.asciidoc

@@ -924,6 +924,15 @@ secondary road endpoint nodes to the nearest reference road.
 
 List of tags to ignore when performing differential conflation with tags.
 
+=== differential.treat.reviews.as.matches
+
+* Data Type: bool
+* Default Value: `true`
+
+If true reviews are treated as matches by Differential Conflation and removed from the output if
+differential.remove.reference.data is enabled. If set to false, reviews are not treated as matches
+and will pass through to the differential output.
+
 === direction.finder.angle.threshold
 
 * Data Type: double
@@ -4316,14 +4325,30 @@ Minimum tag numeric value that will allow the TagValueNumericRangeCriterion to b
 For commands supporting it, the iteration count at which a status message should be logged. This
 setting may have a negative impact on performance if set to a very low value.
 
-=== test.case.cmd
+=== test.case.conflate.cmd
 
 * Data Type: string
 * Default Value: `hoot::ConflateCmd`
 
 Set the conflate command that should be used in a test case. This is only useful when writing
 test cases (`test-files/cases/`) and was originally added to support the MultiaryPoiConflateCmd.
 
+=== test.case.conflate.differential
+
+* Data Type: bool
+* Default Value: `false`
+
+When activated, this runs the conflate case test conflate command with the --differential option.
+
+=== test.case.conflate.differential.include.tags
+
+* Data Type: bool
+* Default Value: `false`
+
+When activated, this runs the conflate case test conflate command with both the --differential
+and --include-tags options (setting this to true automatically sets test.case.conflate.differential
+to true).
+
 === test.force.orthographic.projection
 
 * Data Type: bool

conf/services/conflationTypes.json

@@ -27,6 +27,7 @@
         "members": {
             "differential.remove.unconflatable.data": "Pass unconflatable data from the secondary input to output",
             "differential.snap.unconnected.roads": "Snap unconnected secondary roads to reference roads",
+	    "differential.treat.reviews.as.matches": "Treat reviews as matches and remove from output",
             "snap.unconnected.ways.snap.tolerance": "Maximum distance, in meters, to allow snapping unconnected roads to neighboring roads",
             "snap.unconnected.ways.use.existing.way.nodes": "Reuse highway nodes when snapping unconnected roads",
             "snap.unconnected.ways.existing.way.node.tolerance": "Maximum distance, in meters, to allow snapping unconnected highway nodes to neighboring roads"

hoot-core-test/src/test/cpp/hoot/core/test/ConflateCaseTest.cpp

@@ -72,6 +72,22 @@ void ConflateCaseTest::_runConflateCmd()
   args << in1.absoluteFilePath();
   args << in2.absoluteFilePath();
   args << testOutput;
+  bool differential = ConfigOptions().getTestCaseConflateDifferential();
+  const bool differentialWithTags = ConfigOptions().getTestCaseConflateDifferentialIncludeTags();
+  if (differentialWithTags)
+  {
+    // let this override and correct what would otherwise be an invalid config
+    differential = true;
+  }
+  if (differential)
+  {
+    args << "--differential";
+  }
+  if (differentialWithTags)
+  {
+    args << "--include-tags";
+  }
+
   int result = -1;
   try
   {
@@ -85,8 +101,8 @@ void ConflateCaseTest::_runConflateCmd()
   QFileInfo expected(_d, "Expected.osm");
   if (expected.exists() == false)
   {
-    throw IllegalArgumentException("Unable to find Expected.osm in conflate case: " +
-      _d.absolutePath());
+    throw IllegalArgumentException(
+      "Unable to find Expected.osm in conflate case: " + _d.absolutePath());
   }
 
   if (result != 0)
@@ -176,11 +192,11 @@ void ConflateCaseTest::runTest()
   // configures and cleans up the conf() environment
   TestSetup st(_confs);
 
-  if (ConfigOptions().getTestCaseCmd().toStdString() == ConflateCmd::className())
+  if (ConfigOptions().getTestCaseConflateCmd().toStdString() == ConflateCmd::className())
   {
     _runConflateCmd();
   }
-  else if (ConfigOptions().getTestCaseCmd() == multiaryConflateClass)
+  else if (ConfigOptions().getTestCaseConflateCmd() == multiaryConflateClass)
   {
     _runMultiaryConflateCmd();
   }

hoot-core/src/main/cpp/hoot/core/algorithms/linearreference/NaiveWayMatchStringMapping.h

@@ -37,6 +37,7 @@ class WayString;
 class NaiveWayMatchStringMapping : public WayMatchStringMapping
 {
 public:
+
   NaiveWayMatchStringMapping(WayStringPtr str1, WayStringPtr str2);
 
   virtual WayStringPtr getWayString1() { return _ws1; }
@@ -52,6 +53,7 @@ class NaiveWayMatchStringMapping : public WayMatchStringMapping
   virtual void setWayString2(const WayStringPtr& ws2) { _ws2 = ws2; }
 
 private:
+
   WayStringPtr _ws1, _ws2;
   Meters _length1, _length2;
 };

hoot-core/src/main/cpp/hoot/core/algorithms/linearreference/WayMatchStringMapping.h

@@ -87,6 +87,9 @@ class WayMatchStringMapping
   virtual void setWayString2(const WayStringPtr& ws2) = 0;
   void setWayString(WayNumber way, const WayStringPtr& ws)
   { (way == WayNumber::Way1) ? setWayString1(ws) : setWayString2(ws); }
+
+  QString toString()
+  { return "1: " + getWayString1()->toString() + "; 2: " + getWayString2()->toString(); }
 };
 
 typedef std::shared_ptr<WayMatchStringMapping> WayMatchStringMappingPtr;

hoot-core/src/main/cpp/hoot/core/algorithms/optimizer/IntegerProgrammingSolver.cpp

@@ -22,7 +22,7 @@
  * This will properly maintain the copyright information. DigitalGlobe
  * copyrights will be updated automatically.
  *
- * @copyright Copyright (C) 2015, 2017, 2018 DigitalGlobe (http://www.digitalglobe.com/)
+ * @copyright Copyright (C) 2015, 2017, 2018, 2019 DigitalGlobe (http://www.digitalglobe.com/)
  */
 #include "IntegerProgrammingSolver.h"
 
@@ -82,6 +82,8 @@ void IntegerProgrammingSolver::solve()
 
 void IntegerProgrammingSolver::solveBranchAndCut()
 {
+  LOG_DEBUG("solveBranchAndCut");
+
   glp_iocp iocp;
   glp_init_iocp(&iocp);
   //  Turn on the presolver so that glp_intopt works correctly
@@ -133,6 +135,8 @@ void IntegerProgrammingSolver::solveBranchAndCut()
 
 void IntegerProgrammingSolver::solveSimplex()
 {
+  LOG_DEBUG("solveSimplex");
+
   glp_smcp smcp;
   glp_init_smcp(&smcp);
   //  Setup the time limit if necessary

hoot-core/src/main/cpp/hoot/core/cmd/ConflateCmd.cpp

@@ -55,6 +55,8 @@
 #include <hoot/core/visitors/CountUniqueReviewsVisitor.h>
 #include <hoot/core/util/ConfigUtils.h>
 #include <hoot/core/elements/OsmUtils.h>
+#include <hoot/core/ops/RemoveRoundabouts.h>
+#include <hoot/core/ops/ReplaceRoundabouts.h>
 
 // Standard
 #include <fstream>
@@ -163,19 +165,33 @@ int ConflateCmd::runSimple(QStringList& args)
   Progress progress(ConfigOptions().getJobId(), JOB_SOURCE, Progress::JobState::Running);
   const int maxFilePrintLength = ConfigOptions().getProgressVarPrintLengthMax();
   QString msg =
-    "Conflating ..." + input1.right(maxFilePrintLength) + " with ..." +
-    input2.right(maxFilePrintLength) + " and writing the output to ..." +
+    "Conflating " + input1.right(maxFilePrintLength) + " with " +
+    input2.right(maxFilePrintLength) + " and writing the output to " +
     output.right(maxFilePrintLength);
   if (isDiffConflate)
   {
-    msg = msg.prepend("Differentially ");
+    if (diffConflator.conflatingTags())
+    {
+      msg = msg.replace("Conflating", "Differentially conflating (tags only) ");
+    }
+    else
+    {
+      msg = msg.replace("Conflating", "Differentially conflating ");
+    }
   }
+
   progress.set(0.0, msg);
 
   double bytesRead = IoSingleStat(IoSingleStat::RChar).value;
   LOG_VART(bytesRead);
   QList<QList<SingleStat>> allStats;
 
+  _updateConfigOptionsForAttributeConflation();
+  if (isDiffConflate)
+  {
+    _updateConfigOptionsForDifferentialConflation();
+  }
+
   // The number of steps here must be updated as you add/remove job steps in the logic.
   _numTotalTasks = 5;
   if (displayStats)
@@ -334,7 +350,6 @@ int ConflateCmd::runSimple(QStringList& args)
   stats.append(SingleStat("Conflation Time (sec)", t.getElapsedAndRestart()));
   currentTask++;
 
-  _updatePostConfigOptionsForAttributeConflation();
   if (ConfigOptions().getConflatePostOps().size() > 0)
   {
     // apply any user specified post-conflate operations
@@ -476,7 +491,32 @@ float ConflateCmd::_getJobPercentComplete(const int currentTaskNum) const
   return (float)currentTaskNum / (float)_numTotalTasks;
 }
 
-void ConflateCmd::_updatePostConfigOptionsForAttributeConflation()
+void ConflateCmd::_updateConfigOptionsForDifferentialConflation()
+{
+  // Since Differential throws out all matches, there's no way we can have a bad merge between
+  // ref/secondary roundabouts. Therefore, no need to replace/remove them. If there's a match, we'll
+  // end with no secondary roundabout in the diff output and only the ref roundabout when the diff
+  // is applied back to the ref.
+
+  QStringList preConflateOps = ConfigOptions().getConflatePreOps();
+  const QString removeRoundaboutsClassName = QString::fromStdString(RemoveRoundabouts::className());
+  if (preConflateOps.contains(removeRoundaboutsClassName))
+  {
+    preConflateOps.removeAll(removeRoundaboutsClassName);
+    conf().set(ConfigOptions::getConflatePreOpsKey(), preConflateOps);
+  }
+
+  QStringList postConflateOps = ConfigOptions().getConflatePostOps();
+  const QString replaceRoundaboutsClassName =
+    QString::fromStdString(ReplaceRoundabouts::className());
+  if (postConflateOps.contains(replaceRoundaboutsClassName))
+  {
+    postConflateOps.removeAll(replaceRoundaboutsClassName);
+    conf().set(ConfigOptions::getConflatePostOpsKey(), postConflateOps);
+  }
+}
+
+void ConflateCmd::_updateConfigOptionsForAttributeConflation()
 {
   // These are some custom adjustments to config opts that must be done for Attribute Conflation.
   // There may be a way to eliminate these by adding more custom behavior to the UI.

hoot-core/src/main/cpp/hoot/core/cmd/ConflateCmd.h

@@ -67,7 +67,8 @@ class ConflateCmd : public BaseCommand
 
   int _numTotalTasks;
 
-  void _updatePostConfigOptionsForAttributeConflation();
+  void _updateConfigOptionsForAttributeConflation();
+  void _updateConfigOptionsForDifferentialConflation();
   void _checkForTagValueTruncationOverride();
 
   float _getJobPercentComplete(const int currentTaskNum) const;

hoot-core/src/main/cpp/hoot/core/conflate/DiffConflator.cpp

@@ -125,7 +125,7 @@ void DiffConflator::apply(OsmMapPtr& map)
 
   _updateProgress(currentStep - 1, "Matching features...");
 
-  // If we don't do this, then any non-matchable data will simply pass through to output.
+  // If we skip this part, then any non-matchable data will simply pass through to output.
   if (ConfigOptions().getDifferentialRemoveUnconflatableData())
   {
     LOG_INFO("Discarding unconflatable elements...");
@@ -161,9 +161,8 @@ void DiffConflator::apply(OsmMapPtr& map)
 
   currentStep++;
 
-  // Use matches to calculate and store tag diff. We must do this before we
-  // create the map diff, because that operation deletes all of the info needed
-  // for calculating the tag diff.
+  // Use matches to calculate and store tag diff. We must do this before we create the map diff,
+  // because that operation deletes all of the info needed for calculating the tag diff.
   _updateProgress(currentStep - 1, "Storing tag differentials...");
   _calcAndStoreTagChanges();
   currentStep++;
@@ -219,28 +218,37 @@ void DiffConflator::_snapSecondaryRoadsBackToRef()
 void DiffConflator::_removeMatches(const Status& status)
 {
   LOG_DEBUG("\tRemoving match elements with status: " << status.toString() << "...");
+
+  const bool treatReviewsAsMatches = ConfigOptions().getDifferentialTreatReviewsAsMatches();
+  LOG_VARD(treatReviewsAsMatches);
   for (std::vector<ConstMatchPtr>::iterator mit = _matches.begin(); mit != _matches.end(); ++mit)
   {
-    std::set<std::pair<ElementId, ElementId>> pairs = (*mit)->getMatchPairs();
-    for (std::set<std::pair<ElementId, ElementId>>::iterator pit = pairs.begin();
-         pit != pairs.end(); ++pit)
+    ConstMatchPtr match = *mit;
+    if (treatReviewsAsMatches || match->getType() != MatchType::Review)
     {
-      if (!pit->first.isNull())
+      std::set<std::pair<ElementId, ElementId>> pairs = (*mit)->getMatchPairs();
+      for (std::set<std::pair<ElementId, ElementId>>::iterator pit = pairs.begin();
+           pit != pairs.end(); ++pit)
       {
-        LOG_VART(pit->first);
-        ElementPtr e = _pMap->getElement(pit->first);
-        if (e && e->getStatus() == status)
+        if (!pit->first.isNull())
         {
-          RecursiveElementRemover(pit->first).apply(_pMap);
+          LOG_VART(pit->first);
+          ElementPtr e = _pMap->getElement(pit->first);
+          if (e && e->getStatus() == status)
+          {
+            //LOG_VART(e->getTags().get("name"));
+            RecursiveElementRemover(pit->first).apply(_pMap);
+          }
         }
-      }
-      if (!pit->second.isNull())
-      {
-        LOG_VART(pit->second);
-        ElementPtr e = _pMap->getElement(pit->second);
-        if (e && e->getStatus() == status)
+        if (!pit->second.isNull())
         {
-          RecursiveElementRemover(pit->second).apply(_pMap);
+          LOG_VART(pit->second);
+          ElementPtr e = _pMap->getElement(pit->second);
+          if (e && e->getStatus() == status)
+          {
+            //LOG_VART(e->getTags().get("name"));
+            RecursiveElementRemover(pit->second).apply(_pMap);
+          }
         }
       }
     }
@@ -326,21 +334,18 @@ void DiffConflator::addChangesToMap(OsmMapPtr pMap, ChangesetProviderPtr pChange
     }
     else if (ElementType::Relation == c.getElement()->getElementType().getEnum())
     {
-      // Diff conflation doesn't do relations yet
+      // Diff conflation w/ tags doesn't handle relations. Changed this to silently log that the
+      // relations are being skipped for now. #3449 was created to deal with adding relation support
+      // and then closed since we lack a use case currently that requires it. If we ever get one,
+      // then we can re-open that issue.
 
-      if (logWarnCount < Log::getWarnMessageLimit())
+      LOG_DEBUG("Relation handling not implemented with differential conflation: " << c);
+      if (Log::getInstance().getLevel() <= Log::Trace)
       {
-        LOG_WARN("Relation handling not implemented with differential conflation: " << c);
-        LOG_VART(c);
         ConstRelationPtr relation = std::dynamic_pointer_cast<const Relation>(c.getElement());
         LOG_VART(relation->getElementId());
         LOG_VART(OsmUtils::getRelationDetailedString(relation, _pOriginalMap));
       }
-      else if (logWarnCount == Log::getWarnMessageLimit())
-      {
-        LOG_WARN(className() << ": " << Log::LOG_WARN_LIMIT_REACHED_MESSAGE);
-      }
-      logWarnCount++;
     }
   }
   OsmMapWriterFactory::writeDebugMap(pMap, "after-adding-diff-tag-changes");
@@ -394,13 +399,16 @@ void DiffConflator::_calcAndStoreTagChanges()
       }
 
       LOG_VART(pOldElement->getElementId());
+      //LOG_VART(pOldElement->getTags().get("name"));
       LOG_VART(pNewElement->getElementId());
+      //LOG_VART(pNewElement->getTags().get("name"));
 
-      // Apparently a NetworkMatch can be a node/way pair. See note in
+      // Apparently, a NetworkMatch can be a node/way pair. See note in
       // NetworkMatch::_discoverWayPairs as to why its allowed. However, tag changes between
       // node/way match pairs other than poi/poly don't seem to make any sense. Clearly, if we add
-      // other conflation type other than poi/poly which matches differing geometry types then this
-      // will need to be updated.
+      // a conflation type other than poi/poly which matches differing geometry types then this will
+      // need to be updated.
+
       if (match->getMatchName() != PoiPolygonMatch().getMatchName() &&
           pOldElement->getElementType() != pNewElement->getElementType())
       {