3387 - Add option to diff conflate to keep reviews in output and more (#3588)

* Added an option, `differential.treat.reviews.as.matches`, that allows for not treating reviews as matches in Differential Conflation, which will let them pass to the diff output. The use case for this was a situation where a one to many POI review was preventing several secondary POIs from being added to the diff. Since some of these reviews were questionable, the longer term solution to this is some reworking of POI reviews (#3579). In the meantime, this config option may be useful.
* Disabled the removal/replacement of roundabouts during Differential Conflation. in #3580 there was a situation where roundabouts were being mangled badly during roundabout replacement. The original point of roundabout removal/replacement was to preserve them, since we don't conflate them very well some of the time. However with diff conflate, there's no chance to mangle them since we're only keeping non-matches in the secondary data and not actually trying to merge ref and secondary roundabouts...so removing the logic was the simplest solution.
* Downgraded warning logged when diff conflate w/ tags encounters a relation to a debug statement, since that is normal operation for now. #3449 can be re-opened to handle relations if need be.
* Fixed a NPE in the Network alg when running ref conflate with the same data from #3387
* Fixed issue where diff conflate case tests were not actually running diff conflate
* Fxied handling of output size limit in `MapComparator::_printIdDiff`
* Added in some utilities to `Roundabout`, `OsmUtils`, `Node`, and `Way` that make debugging roundabout conflate problems easier

Author: bwitham

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())
       {