osmlab / atlas-checks

OSM data integrity checks with Atlas
BSD 3-Clause "New" or "Revised" License
57 stars 82 forks source link

[Fix Suggestions] Malformed Roundabout Check #387

Closed ylinzhi closed 3 years ago

ylinzhi commented 4 years ago

Which Atlas check is the fix suggestion related to?

Malformed Roundabout Check

Description: https://github.com/osmlab/atlas-checks/blob/dev/docs/checks/malformedRoundaboutCheck.md Script: https://github.com/osmlab/atlas-checks/blob/dev/src/main/java/org/openstreetmap/atlas/checks/validation/linear/edges/MalformedRoundaboutCheck.java

Describe more details of suggested fix(es)

Suggested Fix 1:

Is there any code enhancement needed if adding the fix suggestion component?

No

seancoulter commented 4 years ago

Looks good. We should be able to suggest auto fixes for roundabouts with invalid source edges (must be a main edge with an ISO code and junction=roundabout) in addition to the other 2 cases mentioned

vladlemberg commented 3 years ago

The implementation of AutoFix turn out not a straightforward task. Proposed error checks occurs in different codebase: ComplexRoundabout (Atlas). MalformedRoundabout received combined error messages:

WRONG_WAY_INVALIDATION = "This roundabout is going the wrong direction, or has been improperly tagged as a roundabout.";

INCOMPLETE_ROUTE_INVALIDATION = "This roundabout does not form a single, one-way, complete, car navigable route.";

In order to isolate each particular case, ComplexRoundabout error messaging has to be more granular. Please refer: https://github.com/osmlab/atlas/issues/709.

@ylinzhi @seancoulter @Bentleysb @sayas01 @atiannicelli

vladlemberg commented 3 years ago

@Bentleysb, I'd like to clarify AutoFix suggestion scenarios based on our discussion in PR. Wrong Roundabout Direction Case 1: Simple Roundabout. Closed way. 4 Atlas Edges. Example Requirements: Create single AutoFix edit comprising 4 Edges

Case 2: Complex Roundabout. Unclosed Way. Different OSM ids. Example: need to find Requirements: Create multiple AutoFix edits per each OSM id. Am I understand this correctly ?

Question: is there any way to deferentiate these two cases in ComplexRoundabout ?

@sayas01 could you please check original requirement document to find example for case 2 ?

Bentleysb commented 3 years ago

@vladlemberg Here is an example of a multi way roundabout: https://www.openstreetmap.org/way/349400768

I'm not sure there is a way to differentiate these two cases in ComplexRoundabout, but they can certainly be differentiated in the check by getting the number of unique OSM IDs in the roundaboutEdgeSet.

However, I don't think there is any need for differentiation. In both cases we want to both flag and create fix suggestion for each edge in roundaboutEdgeSet. No matter the amount of Ways in the roundabout or the amount of times they are way sectioned into Edges, this will ensure that all the Atlas and OSM features that make up the roundabout are flagged and that the entire geometry is reversed through the combination of all the fix suggestions.

The end result that we are looking for with the fix suggestions, in both cases, is that each edge has an individual fix suggestion to reverse its geometry as part of the CheckFlag.

vladlemberg commented 3 years ago

@Bentleysb, thanks for the example!

To clarify Case 1: single way roundabout -> create 1 AutoFix suggestion comprising all sectioned Edges Case2: multi way roundabout -> create AutoFix suggestion per unique OSM ID

Possible false positives for case 2. Assuming that only one section of multi way roundabout has wrong direction. Because we don't know which one it is, we will flag all of them.

Proposing the logic: Case 1: single way roundabout -> single AutoFix: mark all edges as flagged. Case 2: multi way roundabout -> single AutoFix: do not mark all edges as flagged. This will slightly increate iterations but implementation remains simple.

Thoughts ?

Bentleysb commented 3 years ago

@vladlemberg, that is a good point about the false positive possibility for case 2. If we cannot identify which section(s) are going the wrong directions then we should probably not include a fix suggestion for that case.

For case one, and case two if we know all sections are reversed, we need one fix suggestion per Edge in the roundabout, regardless of OSM ids. One fix suggestion can only ever contain one Atlas object. However a CheckFlag can contain multiple fix suggestions. So for a roundabout made up of 4 Edges we would want a CheckFlag with the 4 Edges flagged and 4 fix suggestions (one to reverse the geometry of each Edge.

I don't think there is any case where we would not want to flag all the Edges in the roundabout in one CheckFlag. If for case two we can identify which ways/sections are reversed then we would still want to flag all the Edges in the roundabout, but we would only have fix suggestions for the reversed Edges. Again it would be one fix suggestion per reversed Edge, with all the fix suggestions being added to a single CheckFlag.

vladlemberg commented 3 years ago

got it. Thanks Bentley!

vladlemberg commented 3 years ago

@Bentleysb, @sayas01, @seancoulter I removed Multi Way roundabout from Auto Fix per our discussion. With respect to creating individual AutoFix per Edge. Since "Single Way" roundabout is complete entity in OSM, I created one AutoFix per roundabout with complete geometry. I had to use OsmWalker instead of roundaboutEdgeSet because its sorted by Atlas Section. I also moved buildOriginalOsmWayGeometry to utility.CommonMethods because it will be used in multiple Checks.

Bentleysb commented 3 years ago

Completed by https://github.com/osmlab/atlas-checks/pull/449.