Incorrectly added highway node tags to changeset using Diff conflate w/ tags and separate output

[bwitham]

https://github.com/DigitalGlobe/VGI-team-repo/issues/1955

[bwitham]

I was able to reproduce this behavior on the nightly test server. Now, need to reproduce it locally for debugging.

[bwitham]

I was just able to reproduce this locally. Previously I wasn't doing separate output for diff tags and geometries. After doing that, I see the issue. I've never worked with the diff separate output changesets before, so tomorrow will study up on how that works.

[bwitham]

hoot convert -D convert.ops="hoot::AddAttributesVisitor" -D add.attributes.visitor.element.criteria="hoot::StatusCriterion" -D status.criterion.status=Unknown1 -D add.attributes.visitor.kvps="version=1" ~/hoot/tmp/NGOR_data_ticket/NOME_data_NGOR.osm ~/hoot/tmp/NGOR_data_ticket/NOME_data_NGOR-version-updated.osm

hoot conflate -C DifferentialConflation.conf -C NetworkAlgorithm.conf -D log.class.filter= ~/hoot/tmp/NGOR_data_ticket/NOME_data_NGOR-version-updated.osm ~/hoot/tmp/NGOR_data_ticket/OSM_data_NGOR.osm ~/hoot/tmp/3386-out-1.osc --differential --include-tags --separate-output

[bwitham]

This seems to be a bug in the network alg matching. A highway node is being matched to a highway, which should never happen. I vaguely remember a problem like this in the past, so will review old commits.

[bwitham]

About a year and a half ago a similar issue was experienced with poi/poly matching, which caused the merging to crash. It seems that the original issues was never tracked down and a band-aid of sorts on ScriptMerger was the best we could do.

I'll try to track it down with the Network matches, but if I can't find what's causing it a band-aid on DiffConflator will have to suffice.

[bwitham]

I found that NetworkMatch is allowing node/way match pairs. How the nodes are getting in there, I'm not sure, since NetworkMatchCreator does a candidate check for highways. I don't see any scenario where a Network alg node/way match pair makes sense, and restricting them to ways only doesn't lower any regression tests scores and seems to reduce some unnecessary reviews in all tests. I still need to go through the resultant failing network tests and make sure the output looks good after the change.

[bwitham]

Failing unit tests:

• attribute/network/highway-2927-name-preservation-1 - OUTPUT BETTER
• attribute/network/highway-2927-unmarked-ref-divided-road-1 - OUTPUT UNCHANGED
• attribute/network/highway-2927-multiple-types-1 - OUTPUT BETTER
• attribute/network/highway-2927-bridge-2 - OUTPUT UNCHANGED
• network/iterative/highway-010 - OUTPUT WORSE
• network/iterative/highway-011 - OUTPUT WORSE
• network/conflicts/highway-008 - OUTPUT WORSE
• network/conflicts/highway-010 - OUTPUT WORSE
• network/conflicts/highway-011 - OUTPUT WORSE
• network/conflicts/highway-030 - OUTPUT WORSE?
• network/conflicts/pap-006 - OUTPUT BETTER
• roundabouts/rcase-001 - OUTPUT BETTER
• network/conflicts/pap-007 - OUTPUT UNCHANGED
• network/conflicts/pap-002 - OUTPUT UNCHANGED
• NetworkConflateCmdInputReverseTest - OUTPUT UNCHANGED
• NetworkConflateCmdTest - OUTPUT UNCHANGED
• NetworkConflateCmdRelationEdgeTest - OUTPUT UNCHANGED
• DiffConflateCmdTest - OUTPUT WORSE (SNAPPED ONLY)

[bwitham]

So, there must be a reason to include nodes in the match pairs after all. Although some tests have better output with the change, there are some that do not. Particularly, the conflicts/highway tests have worse snapping at intersections with the change, which I don't like.

I'm going to go with plan B, and that is to allow the node/way matches in NetworkMatch, but prevent them from triggering tag changes in DiffConflator.