Skip to content

Commit

Permalink
Added fix suggestion to RoadNameGapCheck (osmlab#475)
Browse files Browse the repository at this point in the history 
* Added fix suggestion for RoadNameGapCheck

* Fix suggestion: Store previous name in old_name tag

* Complex fix suggestion changes

- if bridge and no bridge:name, store old name value under it
- otherwise store the old value under alt_name (with increased
index if already existing eg. alt_name_1)

* Add tunnel condition in fix suggestion

Co-authored-by: atiannicelli <[email protected]>

* Added missing TunnelTag import

* code smell

Co-authored-by: atiannicelli <[email protected]>
Co-authored-by: Sean Coulter <[email protected]>
  • Loading branch information
@mm-ciub @atiannicelli
3 people authored Jan 27, 2021
1 parent 185374e commit 3e3d0fd
Show file tree
Hide file tree
Showing 4 changed files with 163 additions and 12 deletions.
8 changes: 6 additions & 2 deletions docs/checks/RoadNameGapCheck.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,18 @@ i) This check should flag the edges who has NO name tag with below conditions:
1. TERTIARY_LINK, SECONDARY_LINK, PRIMARY_LINK, TRUNK_LINK, or MOTORWAY_LINK
c. junction=ROUNDABOUT

For this case a fix suggestion to add the name tag with the same value as the connected edges is issued.

ii) This check should flag the edges which has different Name Tag:
1. Is an Edge
2. Has the Tag,
1. highway=TERTIARY, SECONDARY, PRIMARY, TRUNK, or MOTORWAY.
3. name=* but is not the same as the other Edges it is connected to
4. Does NOT have the Tag,
a. highway=*_LINK
b. junction=ROUNDABOUT
a. highway=*_LINK
b. junction=ROUNDABOUT

For this case a fix suggestion to modify the name tag with the value from the connected edges is issued.

### Live Examples

Expand Down
65 changes: 57 additions & 8 deletions src/main/java/org/openstreetmap/atlas/checks/validation/tag/RoadNameGapCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,23 @@
import java.util.stream.Collectors;

import org.openstreetmap.atlas.checks.base.BaseCheck;
import org.openstreetmap.atlas.checks.constants.CommonConstants;
import org.openstreetmap.atlas.checks.flag.CheckFlag;
import org.openstreetmap.atlas.geography.atlas.change.FeatureChange;
import org.openstreetmap.atlas.geography.atlas.complete.CompleteEdge;
import org.openstreetmap.atlas.geography.atlas.complete.CompleteEntity;
import org.openstreetmap.atlas.geography.atlas.items.AtlasEntity;
import org.openstreetmap.atlas.geography.atlas.items.AtlasObject;
import org.openstreetmap.atlas.geography.atlas.items.Edge;
import org.openstreetmap.atlas.tags.BridgeTag;
import org.openstreetmap.atlas.tags.HighwayTag;
import org.openstreetmap.atlas.tags.JunctionTag;
import org.openstreetmap.atlas.tags.TunnelTag;
import org.openstreetmap.atlas.tags.annotations.validation.Validators;
import org.openstreetmap.atlas.tags.names.AlternativeNameTag;
import org.openstreetmap.atlas.tags.names.BridgeNameTag;
import org.openstreetmap.atlas.tags.names.NameTag;
import org.openstreetmap.atlas.tags.names.TunnelNameTag;
import org.openstreetmap.atlas.utilities.configuration.Configuration;
import org.openstreetmap.atlas.utilities.direction.EdgeDirectionComparator;

Expand All @@ -21,6 +32,7 @@
* Tag, OR the Edge has a name Tag but does not equal the name Tag of the Edges that it is between.
*
* @author sugandhimaheshwaram
* @author mm-ciub
*/
public class RoadNameGapCheck extends BaseCheck<Long>
{
Expand Down Expand Up @@ -113,16 +125,23 @@ protected Optional<CheckFlag> flag(final AtlasObject object)
outEdges);
if (matchingInAndOutEdgeNames.isEmpty())
{
// There is no pair of inedge and out edge with same name.
// There is no pair of in edge and out edge with same name.
return Optional.empty();
}

final String nameSuggestion = matchingInAndOutEdgeNames.iterator().next();
// Create flag when we have in edge and out edge with same name but intermediate edge
// doesn't have a name.
if (!edge.getName().isPresent())
if (edge.getName().isEmpty())
{
return Optional.of(this.createFlag(object,
this.getLocalizedInstruction(0, edge.getOsmIdentifier())));
return Optional
.of(this.createFlag(object,
this.getLocalizedInstruction(0, edge.getOsmIdentifier()))
.addFixSuggestion(
FeatureChange.add(
(AtlasEntity) ((CompleteEntity) CompleteEntity
.from((AtlasEntity) object)).withAddedTag(
NameTag.KEY, nameSuggestion),
object.getAtlas())));
}
final Optional<String> edgeName = edge.getName();

Expand All @@ -132,9 +151,12 @@ protected Optional<CheckFlag> flag(final AtlasObject object)
.filter(this::validCheckForObject).collect(Collectors.toSet());
return this.findMatchingEdgeNameWithConnectedEdges(connectedEdges, edgeName.get())
? Optional.empty()
: Optional.of(this.createFlag(object, this.getLocalizedInstruction(1,
edge.getOsmIdentifier(), edgeName.get())));

: Optional.of(this
.createFlag(object,
this.getLocalizedInstruction(1, edge.getOsmIdentifier(),
edgeName.get()))
.addFixSuggestion(this.getComplexFixSuggestion(object, edgeName.get(),
nameSuggestion)));
}
return Optional.empty();
}
Expand Down Expand Up @@ -162,6 +184,33 @@ private boolean findMatchingEdgeNameWithConnectedEdges(final Set<Edge> connected
&& connectedEdge.getName().get().equals(edgeName));
}

private FeatureChange getComplexFixSuggestion(final AtlasObject object,
final String originalName, final String nameSuggestion)
{
final CompleteEntity<CompleteEdge> completeEntity = (CompleteEntity) CompleteEntity
.from((AtlasEntity) object);
completeEntity.withReplacedTag(NameTag.KEY, NameTag.KEY, nameSuggestion);
if (BridgeTag.isBridge(object) && object.getTag(BridgeNameTag.KEY).isEmpty())
{
completeEntity.withAddedTag(BridgeNameTag.KEY, originalName);
}
else if (TunnelTag.isTunnel(object) && object.getTag(TunnelNameTag.KEY).isEmpty())
{
completeEntity.withAddedTag(TunnelNameTag.KEY, originalName);
}
else
{
final long altNameCount = object.getOsmTags().keySet().stream()
.filter(key -> key.startsWith(AlternativeNameTag.KEY)).count();
final String altNameTag = altNameCount > 0
? (AlternativeNameTag.KEY + CommonConstants.UNDERSCORE + altNameCount)
: AlternativeNameTag.KEY;
completeEntity.withAddedTag(altNameTag, originalName);

}
return FeatureChange.add((AtlasEntity) completeEntity, object.getAtlas());
}

/**
* Find matching in and out edge names by comparing every name of the edge.
*
Expand Down
46 changes: 44 additions & 2 deletions src/test/java/org/openstreetmap/atlas/checks/validation/tag/RoadNameGapCheckTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
import org.junit.Test;
import org.openstreetmap.atlas.checks.configuration.ConfigurationResolver;
import org.openstreetmap.atlas.checks.validation.verifier.ConsumerBasedExpectedCheckVerifier;
import org.openstreetmap.atlas.tags.names.AlternativeNameTag;
import org.openstreetmap.atlas.tags.names.BridgeNameTag;
import org.openstreetmap.atlas.utilities.configuration.Configuration;

/**
Expand All @@ -23,12 +25,48 @@ public class RoadNameGapCheckTest
private final Configuration inlineConfiguration = ConfigurationResolver.inlineConfiguration(
"{\"RoadNameGapCheck\":{\"valid.highway.tag\":[\"primary\",\"tertiary\",\"trunk\",\"motorway\",\"secondary\"]}}");

@Test
public void testForBridgeEdgeWithDifferentNameTag()
{
this.verifier.actual(this.setup.getBridgeEdge(),
new RoadNameGapCheck(this.inlineConfiguration));
this.verifier.globallyVerify(flags ->
{
Assert.assertEquals(1, flags.size());
Assert.assertEquals(1, flags.get(0).getFixSuggestions().size());
flags.get(0).getFixSuggestions()
.forEach(s -> Assert.assertTrue(s.getTag(BridgeNameTag.KEY).isPresent()));
});
}

@Test
public void testForEdgeWithAltName()
{
this.verifier.actual(this.setup.getEdgeWithAltName(),
new RoadNameGapCheck(this.inlineConfiguration));
this.verifier.globallyVerify(flags ->
{
Assert.assertEquals(1, flags.size());
Assert.assertEquals(1, flags.get(0).getFixSuggestions().size());
flags.get(0).getFixSuggestions()
.forEach(s -> Assert.assertTrue(s.getTag(AlternativeNameTag.KEY).isPresent()));
flags.get(0).getFixSuggestions().forEach(
s -> Assert.assertTrue(s.getTag(AlternativeNameTag.KEY + "_1").isPresent()));
});
}

@Test
public void testForEdgeWithDifferentNameTag()
{
this.verifier.actual(this.setup.getEdgeWithDifferentNameTag(),
new RoadNameGapCheck(this.inlineConfiguration));
this.verifier.globallyVerify(flags -> Assert.assertEquals(1, flags.size()));
this.verifier.globallyVerify(flags ->
{
Assert.assertEquals(1, flags.size());
Assert.assertEquals(1, flags.get(0).getFixSuggestions().size());
flags.get(0).getFixSuggestions()
.forEach(s -> Assert.assertTrue(s.getTag(AlternativeNameTag.KEY).isPresent()));
});
}

@Test
Expand All @@ -44,7 +82,11 @@ public void testForEdgeWithNoNameTag()
{
this.verifier.actual(this.setup.getEdgeWithNoNameTag(),
new RoadNameGapCheck(this.inlineConfiguration));
this.verifier.globallyVerify(flags -> Assert.assertEquals(1, flags.size()));
this.verifier.globallyVerify(flags ->
{
Assert.assertEquals(1, flags.size());
Assert.assertEquals(1, flags.get(0).getFixSuggestions().size());
});
}

@Test
Expand Down
56 changes: 56 additions & 0 deletions src/test/java/org/openstreetmap/atlas/checks/validation/tag/RoadNameGapCheckTestRule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,52 @@ public class RoadNameGapCheckTestRule extends CoreTestRule
private static final String TEST_7 = "9.8344387,56.0243522";
private static final String TEST_8 = "9.8343536,56.0243326";

@TestAtlas(
// nodes
nodes = { @Node(coordinates = @Loc(value = TEST_1), id = "0"),
@Node(coordinates = @Loc(value = TEST_2), id = "1"),
@Node(coordinates = @Loc(value = TEST_3), id = "2"),
@Node(coordinates = @Loc(value = TEST_4), id = "3"),
@Node(coordinates = @Loc(value = TEST_5), id = "4"),
@Node(coordinates = @Loc(value = TEST_6), id = "5"),
@Node(coordinates = @Loc(value = TEST_7), id = "6"),
@Node(coordinates = @Loc(value = TEST_8), id = "7") },
// edges
edges = {
@Edge(id = "1001000000", coordinates = { @Loc(value = TEST_5),
@Loc(value = TEST_6) }, tags = { "highway=primary",
"name=Tsing Long Highway" }),
@Edge(id = "1002000001", coordinates = { @Loc(value = TEST_6),
@Loc(value = TEST_7) }, tags = { "highway=primary", "name=failingName",
"bridge=yes" }),
@Edge(id = "1003000002", coordinates = { @Loc(value = TEST_7),
@Loc(value = TEST_8) }, tags = { "highway=primary",
"name=Tsing Long Highway" }) })
private Atlas bridgeEdge;

@TestAtlas(
// nodes
nodes = { @Node(coordinates = @Loc(value = TEST_1), id = "0"),
@Node(coordinates = @Loc(value = TEST_2), id = "1"),
@Node(coordinates = @Loc(value = TEST_3), id = "2"),
@Node(coordinates = @Loc(value = TEST_4), id = "3"),
@Node(coordinates = @Loc(value = TEST_5), id = "4"),
@Node(coordinates = @Loc(value = TEST_6), id = "5"),
@Node(coordinates = @Loc(value = TEST_7), id = "6"),
@Node(coordinates = @Loc(value = TEST_8), id = "7") },
// edges
edges = {
@Edge(id = "1001000000", coordinates = { @Loc(value = TEST_5),
@Loc(value = TEST_6) }, tags = { "highway=primary",
"name=Tsing Long Highway" }),
@Edge(id = "1002000001", coordinates = { @Loc(value = TEST_6),
@Loc(value = TEST_7) }, tags = { "highway=primary", "name=failingName",
"alt_name=firstFailure" }),
@Edge(id = "1003000002", coordinates = { @Loc(value = TEST_7),
@Loc(value = TEST_8) }, tags = { "highway=primary",
"name=Tsing Long Highway" }) })
private Atlas edgeWithAltName;

@TestAtlas(
// nodes
nodes = { @Node(coordinates = @TestAtlas.Loc(value = TEST_3), id = "2"),
Expand Down Expand Up @@ -116,6 +162,16 @@ public class RoadNameGapCheckTestRule extends CoreTestRule
"name=Tsing Long Highway" }) })
private Atlas edgeWithNoNameTag;

public Atlas getBridgeEdge()
{
return this.bridgeEdge;
}

public Atlas getEdgeWithAltName()
{
return this.edgeWithAltName;
}

public Atlas getEdgeWithDifferentNameTag()
{
return this.edgeWithDifferentNameTag;
Expand Down

0 comments on commit 3e3d0fd

Please sign in to comment.