From cd37d9596de7c760d3cc3dfc66be5670c4cbaf67 Mon Sep 17 00:00:00 2001 From: Savannah Ostrowski Date: Mon, 16 Apr 2018 08:43:38 -0700 Subject: [PATCH] Duplicate Point Check (#37) * Initial logic, documentation and tests for DuplicatePoint * Added configuration * Add missing curly brace in config * Updates to unit tests to use TestAtlas * Gradle build * Added missing comma to config * Use pointsAt instead of pointsWithin * Update documentation and flagging * Update for gradle * Only flag one point location --- config/configuration.json | 8 +++ docs/checks/duplicatePointCheck.md | 60 ++++++++++++++++ .../points/DuplicatePointCheck.java | 70 +++++++++++++++++++ .../points/DuplicatePointCheckTest.java | 38 ++++++++++ .../points/DuplicatePointCheckTestRule.java | 38 ++++++++++ 5 files changed, 214 insertions(+) create mode 100644 docs/checks/duplicatePointCheck.md create mode 100644 src/main/java/org/openstreetmap/atlas/checks/validation/points/DuplicatePointCheck.java create mode 100644 src/test/java/org/openstreetmap/atlas/checks/validation/points/DuplicatePointCheckTest.java create mode 100644 src/test/java/org/openstreetmap/atlas/checks/validation/points/DuplicatePointCheckTestRule.java diff --git a/config/configuration.json b/config/configuration.json index ac8e15019..0de4af325 100644 --- a/config/configuration.json +++ b/config/configuration.json @@ -61,6 +61,14 @@ "difficulty": "EASY" } }, + "DuplicatePointCheck": { + "challenge": { + "description": "Tasks contain node locations where duplicate points of interest are found.", + "blurb": "Duplicate Points", + "instruction": "Open your favorite editor and remove one or more of the duplicate points of interest until only one remains.", + "difficulty": "EASY" + } + }, "DuplicateWaysCheck": { "challenge": { "description": "Tasks contain Ways which have been partially or completely duplicated.", diff --git a/docs/checks/duplicatePointCheck.md b/docs/checks/duplicatePointCheck.md new file mode 100644 index 000000000..e242e8cae --- /dev/null +++ b/docs/checks/duplicatePointCheck.md @@ -0,0 +1,60 @@ +# Duplicate Point Check + +#### Description +The Duplicate Point check flags Points in OSM that share the same location. Whether it's a street +light, viewpoint, or tree, a Point represents something that exists at that location, therefore, we +do not want points on top of each other. Also, as per the [One feature, one OSM element principle](https://wiki.openstreetmap.org/wiki/One_feature,_one_OSM_element), +we don't want instances of duplicate Points that represent the same feature. + +#### Live Example +The following examples illustrate two cases where there are two or more Points in the exact same location. +1) This Point [id:538464430](https://www.openstreetmap.org/node/538464430) has two Points in the same +location. +2) This Point [id:768590227](https://www.openstreetmap.org/node/768590227) has two Points in the same +location. + + +#### Code Review +In [Atlas](https://github.com/osmlab/atlas), OSM elements are represented as Edges, Points, Lines, +Nodes & Relations; in our case, we’re working with [Points](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/geography/atlas/items/Point.java). +In OpenStreetMap, the concept of Points does not exist, and are referred to as [Ways](https://wiki.openstreetmap.org/wiki/Node). + +Our first goal in the Duplicate Point check is to validate the incoming Atlas Object. The object: +* Must be a valid Point +* Location of the Point object must not have been seen before + +```java +@Override + public boolean validCheckForObject(final AtlasObject object) + { + return object instanceof Point && !this.isFlagged(((Point) object).getLocation()); + } + +``` + +Next, we get all Points at that Point's location. If there is more than 1 Point object found to be +at that location, we mark the Points as flagged, get all the duplicate Point identifiers at +that location, and flag them. Otherwise, we do not flag the Point. + +```java + @Override + protected Optional flag(final AtlasObject object) + { + final Point point = (Point) object; + + final List duplicates = Iterables + .asList(object.getAtlas().pointsAt(point.getLocation())); + if (duplicates.size() > 1) + { + duplicates.forEach(duplicate -> this.markAsFlagged(point.getLocation())); + final List duplicateIdentifiers = duplicates.stream() + .map(AtlasEntity::getOsmIdentifier).collect(Collectors.toList()); + return Optional.of(this.createFlag(object, + this.getLocalizedInstruction(0, duplicateIdentifiers, point.getLocation()))); + } + + return Optional.empty(); + } +``` +To learn more about the code, please look at the comments in the source code for the check. +[DuplicatePointCheck.java](../../src/main/java/org/openstreetmap/atlas/checks/validation/points/DuplicatePointCheck.java) diff --git a/src/main/java/org/openstreetmap/atlas/checks/validation/points/DuplicatePointCheck.java b/src/main/java/org/openstreetmap/atlas/checks/validation/points/DuplicatePointCheck.java new file mode 100644 index 000000000..2bd792473 --- /dev/null +++ b/src/main/java/org/openstreetmap/atlas/checks/validation/points/DuplicatePointCheck.java @@ -0,0 +1,70 @@ +package org.openstreetmap.atlas.checks.validation.points; + +import java.util.Arrays; +import java.util.List; +import java.util.Optional; +import java.util.stream.Collectors; + +import org.openstreetmap.atlas.checks.base.BaseCheck; +import org.openstreetmap.atlas.checks.flag.CheckFlag; +import org.openstreetmap.atlas.geography.Location; +import org.openstreetmap.atlas.geography.atlas.items.AtlasEntity; +import org.openstreetmap.atlas.geography.atlas.items.AtlasObject; +import org.openstreetmap.atlas.geography.atlas.items.Point; +import org.openstreetmap.atlas.utilities.collections.Iterables; +import org.openstreetmap.atlas.utilities.configuration.Configuration; + +/** + * This check looks for two or more {@link Point}s that are in the exact same location. + * + * @author savannahostrowski + */ +public class DuplicatePointCheck extends BaseCheck +{ + private static final List FALLBACK_INSTRUCTIONS = Arrays + .asList("Nodes {0} are duplicates at {1}"); + private static final long serialVersionUID = 8624313405718452123L; + + @Override + protected List getFallbackInstructions() + { + return FALLBACK_INSTRUCTIONS; + } + + /** + * Default constructor + * + * @param configuration + * the JSON configuration for this check + */ + public DuplicatePointCheck(final Configuration configuration) + { + super(configuration); + } + + @Override + public boolean validCheckForObject(final AtlasObject object) + { + + return object instanceof Point && !this.isFlagged(((Point) object).getLocation()); + } + + @Override + protected Optional flag(final AtlasObject object) + { + final Point point = (Point) object; + + final List duplicates = Iterables + .asList(object.getAtlas().pointsAt(point.getLocation())); + if (duplicates.size() > 1) + { + this.markAsFlagged(point.getLocation()); + final List duplicateIdentifiers = duplicates.stream() + .map(AtlasEntity::getOsmIdentifier).collect(Collectors.toList()); + return Optional.of(this.createFlag(object, + this.getLocalizedInstruction(0, duplicateIdentifiers, point.getLocation()))); + } + + return Optional.empty(); + } +} diff --git a/src/test/java/org/openstreetmap/atlas/checks/validation/points/DuplicatePointCheckTest.java b/src/test/java/org/openstreetmap/atlas/checks/validation/points/DuplicatePointCheckTest.java new file mode 100644 index 000000000..039b9073c --- /dev/null +++ b/src/test/java/org/openstreetmap/atlas/checks/validation/points/DuplicatePointCheckTest.java @@ -0,0 +1,38 @@ +package org.openstreetmap.atlas.checks.validation.points; + +import org.junit.Assert; +import org.junit.Rule; +import org.junit.Test; +import org.openstreetmap.atlas.checks.configuration.ConfigurationResolver; +import org.openstreetmap.atlas.checks.validation.verifier.ConsumerBasedExpectedCheckVerifier; + +/** + * Tests for {@link DuplicatePointCheck} + * + * @author savannahostrowski + */ +public class DuplicatePointCheckTest +{ + @Rule + public DuplicatePointCheckTestRule setup = new DuplicatePointCheckTestRule(); + + @Rule + public ConsumerBasedExpectedCheckVerifier verifier = new ConsumerBasedExpectedCheckVerifier(); + + @Test + public void singlePoint() + { + this.verifier.actual(this.setup.singlePointAtlas(), + new DuplicatePointCheck(ConfigurationResolver.emptyConfiguration())); + this.verifier.globallyVerify(flags -> Assert.assertEquals(0, flags.size())); + } + + @Test + public void duplicatePoint() + { + this.verifier.actual(this.setup.duplicatePointAtlas(), + new DuplicatePointCheck(ConfigurationResolver.emptyConfiguration())); + this.verifier.globallyVerify(flags -> Assert.assertEquals(1, flags.size())); + } + +} diff --git a/src/test/java/org/openstreetmap/atlas/checks/validation/points/DuplicatePointCheckTestRule.java b/src/test/java/org/openstreetmap/atlas/checks/validation/points/DuplicatePointCheckTestRule.java new file mode 100644 index 000000000..a862e52cd --- /dev/null +++ b/src/test/java/org/openstreetmap/atlas/checks/validation/points/DuplicatePointCheckTestRule.java @@ -0,0 +1,38 @@ +package org.openstreetmap.atlas.checks.validation.points; + +import org.openstreetmap.atlas.geography.atlas.Atlas; +import org.openstreetmap.atlas.utilities.testing.CoreTestRule; +import org.openstreetmap.atlas.utilities.testing.TestAtlas; +import org.openstreetmap.atlas.utilities.testing.TestAtlas.Loc; +import org.openstreetmap.atlas.utilities.testing.TestAtlas.Point; + +/** + * {@link DuplicatePointCheckTest} data generator + * + * @author savannahostrowski + */ +public class DuplicatePointCheckTestRule extends CoreTestRule +{ + private static final String TEST_1 = "37.3314171,-122.0304871"; + + @TestAtlas( + // points + points = { @Point(id = "1", coordinates = @Loc(value = TEST_1)) }) + private Atlas singlePointAtlas; + @TestAtlas( + // points + points = { @Point(id = "2", coordinates = @Loc(value = TEST_1)), + @Point(id = "3", coordinates = @Loc(value = TEST_1)) }) + private Atlas duplicatePointAtlas; + + public Atlas singlePointAtlas() + { + return this.singlePointAtlas; + } + + public Atlas duplicatePointAtlas() + { + return this.duplicatePointAtlas; + } + +}