ngageoint / hootenanny

Hootenanny conflates multiple maps into a single seamless map.
GNU General Public License v3.0
357 stars 74 forks source link

Validate changeset output for ref/conflate Milford diff #3851

Closed bwitham closed 4 years ago

bwitham commented 4 years ago

Scrutinize both the conflated output and the changeset, as it seem initially there are far more create/deletes than are needed in the changeset.

Its possible there are some debug tags creeping in that force the changes, although I thought we've already accounted for all of them during element comparison.

bwitham commented 4 years ago

test created. seeing features dropped from ref for no apparent reason...looking into it.

bwitham commented 4 years ago

Found a bug where SuperfluousNodeRemover is removing a node still in a relation.

bwitham commented 4 years ago

Fixed previous. Found another bug: RemoveEmptyAreasVisitor is removing a natural=tree_row ... very strange.

bwitham commented 4 years ago

Fixed natural=tree_row issue by changing its geometry definition in the schema from area to linestring.

bwitham commented 4 years ago

Now seeing that the SuperfluousNodeRemover change reveals a poi/poly match bug... :-(

bwitham commented 4 years ago

Fixed both the above.

Found a dangling roundabout connector that should have been deleted by the ReplaceRoundabouts post conflate op but wasn't. Looking at it.

bwitham commented 4 years ago

Dangling connector was due to IDs changing after conflate merging. Fixed by adding another pass at deleting connectors at the end of roundabout replacement.

bwitham commented 4 years ago

I'm seeing situations where two roads conflate, have two identical geometries, and the secondary adds a name (Northern section of "Ball Park Lane"). There end up being delete/create statements generated for the ref to conflate diff. I think we can get things like this into a single modify statement, so going to look into that now.

bwitham commented 4 years ago

This may be an ID retention issue, b/c if the ref ID had been maintained I think we should have ended up with the modify statement instead of create/delete.

bwitham commented 4 years ago

Merged road assigned a new ID for sure instead of keeping the ref ID...will track down why tomorrow.

bwitham commented 4 years ago

I was mistaken on the two geometries of the roads that were merged being identical. They are slightly different, so I think the particular combo warrants a create/delete after all. Moving on.

bwitham commented 4 years ago

Found another pair of roads that truly look identical, "Oak Hill Drive", and still get a create/delete generated. Going to look at them and see why we can't have a modify instead.

bwitham commented 4 years ago

The match way has the ref parent ID on it, so need to figure out why the parent ID never gets added back as the final ID at the end.

bmarchant commented 4 years ago

After all of the way joining it still has a parent ID? I assume that mean that the parent no longer exists and maybe at the end of the WayJoiner we can iterate all ways with a parent ID where the way no longer exists and make that the ID of the way. Just a simple ID switch. Just a thought.

bwitham commented 4 years ago

Haven't got that far in the debugging yet...not quite sure. Might be what's going on.

bwitham commented 4 years ago

Yeah, I think your suggestion might work. I'm going to try it to see what happens to this road. The matched way was getting its parent ID overwritten in HighwaySnapMerger with an empty one (equal to zero). After I added a check not to overwrite with zero, then I get to the end in WayJoiner and have a null parent. Will see what happens.

bwitham commented 4 years ago

I was able to get that approach to give me the output I wanted for that road after some hair pulling to find the right place to make the change that wouldn't cause crashes to occur. However, it breaks a ton of tests and crashes others. I'll spend some more time tomorrow to see if I can make it work for everything.

bwitham commented 4 years ago

I found another way to get the output with the IDs that I want by checking the roads to be merged up front in the merger, and if they're identical skip all the subline matching parts and just keep the first input. Seems to be working so far. Still going through the tests....some unexplainable output so far, but not as nearly much as initially feared. Have to run regression too.

bwitham commented 4 years ago

Got rid of 100+ delete statements out of ~400 in the Milford AOI.

bwitham commented 4 years ago

Also going to add RemoveDuplicateRelationMembersVisitor to conflate.post.ops to clean up some dupes I'm seeing in the output.

bwitham commented 4 years ago

Now, looking at a dropped section of the road "Gardenia Boulevard" in the conflated output coming from the secondary. There's no ref nearby to conflict with, so not sure yet what could be causing the dropped section.

bwitham commented 4 years ago

DuplicateNodeRemoverRemoveDuplicateWayNodesVisitor is the culprit for Gardenia. Strange, b/c its removing a way node and shouldn't be.

bwitham commented 4 years ago

The way in question is a loop with an extension and the direction changes once you get out of the loop and go into the extension.

bwitham commented 4 years ago

way node issue fixed

Next up are instances of two railway=level_crossing nodes on top of each other that aren't conflating. They should be conflating with either POI or generic point conflation depending up on how the schema is configured.

bwitham commented 4 years ago

correction, the crossings come back as a review. not sure if I can make it into a match or not.

bwitham commented 4 years ago

Leaving POI conflation alone for now, due to the amount of work involved to make changes to it.

Moving on to two highway=turning_circle nodes that should have conflated. They don't classify as POIs, road conflation doesn't handle them, and since they are part of a way generic point conflation won't handle them either. Considering adding a list to PointCriterion for nodes allowed to be in a way node and still be considered points. Will add highway=turning_circle to the list and see what happens.

bwitham commented 4 years ago

Another option would be to elevate highway=turning_circle to POI in the schema...there are plenty of other generic things like that in there. May also tweak railway=crossing for POI after all.

A final option could be to handle the railway crossing as part of the linear railway itself and handle the turning circle as part of the linear road. Even if they're not merged, they need to at least be deleted.

bwitham commented 4 years ago

After thinking about it some more, handling railway=level_crossing as part of linear railway conflation and not part of POI conflation is the right choice, as the crossing nodes will always be connected to the railway.

Also, handling highway=turning_circle as part of road conflation makes sense too. They are guaranteed to be part of the road, just may or may not be at the end. So, some extra logic may have to be added to eliminate duplicated turning circles that end up close together after road merging.

bwitham commented 4 years ago

fixed both the railway crossing and turning circle issues

Moving onto some other tasks but some things to look at later:

bwitham commented 4 years ago

Going to go ahead and close this and open up separate issues for each of the remaining.