Skip to content

Commit

Permalink
Removing superfluous Atlas Point (#48)
Browse files Browse the repository at this point in the history 
* Removing superfluous Atlas Point and tests

* Fixing javadoc warning

* Using text atlas files instead of .osm files to properly test Atlas creation

* undoing text atlas conversion

* Explaining PBF presence as test data
  • Loading branch information
@MikeGost @matthieun
MikeGost authored and matthieun committed Jan 10, 2018
1 parent 9a3740c commit 386cdb2
Show file tree
Hide file tree
Showing 12 changed files with 475 additions and 23 deletions.
83 changes: 62 additions & 21 deletions src/main/java/org/openstreetmap/atlas/geography/atlas/pbf/store/PbfMemoryStore.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@
import org.openstreetmap.osmosis.core.task.v0_6.SinkRunnableSource;
import org.openstreetmap.osmosis.xml.common.CompressionMethod;
import org.openstreetmap.osmosis.xml.v0_6.XmlWriter;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* This {@link PbfMemoryStore} holds all the information needed by country slicing, way sectioning
Expand All @@ -68,6 +70,8 @@ public class PbfMemoryStore implements SinkRunnableSource
private static final TagMapToTagCollectionConverter TAG_MAP_TO_TAG_COLLECTION_CONVERTER = new TagMapToTagCollectionConverter();
private static final CountryListTwoWayStringConverter COUNTRY_LIST_CONVERTER = new CountryListTwoWayStringConverter();

private static final Logger logger = LoggerFactory.getLogger(PbfMemoryStore.class);

private final Map<Long, Node> nodes;
private final Map<Long, Way> ways;
private final Map<Long, Relation> relations;
Expand Down Expand Up @@ -503,30 +507,21 @@ public boolean isAtlasPoint(final long identifier)

public boolean isAtlasPoint(final Node node)
{
if (containsNodeInRelations(node.getId()))
final boolean hasExplicitOsmTags = nodeHasExplicitOsmTags(node);
final long nodeIdentifier = node.getId();

if (containsNodeInRelations(nodeIdentifier) && !hasExplicitOsmTags
&& !isAtlasNode(nodeIdentifier))
{
// When the OSM Node is part of a Relation, doesn't have any explicit OSM tagging and is
// not at an intersection (not an Atlas Node), then we want to create an Atlas Point so
// we don't lose this Node as a member of our Relation.
return true;
}
// All the OSM features will have tags that are added by the Atlas generation: last edit
// time, last user (from PBF) as well as some synthetic boundary tags for the nodes that are
// created at the provided boundary. Because an OSM Node becomes a Point only when it has
// tags, the logic here needs to make sure not to count the synthetic tags to make that
// decision.
// Tags from OSM are the tags that all the nodes will have
if (node.getTags().size() > AtlasTag.TAGS_FROM_OSM.size())
{
int counter = 0;
for (final Tag tag : node.getTags())
{
// Tags from Atlas are the tags that only some nodes will have
if (AtlasTag.TAGS_FROM_ATLAS.contains(tag.getKey()))
{
counter++;
}
}
return node.getTags().size() > AtlasTag.TAGS_FROM_OSM.size() + counter;
}
return false;

// All other times, we defer to the presence of explicit OSM tagging to determine whether
// it's a Point
return hasExplicitOsmTags;
}

public boolean isOneNodeWay(final Way way)
Expand Down Expand Up @@ -740,4 +735,50 @@ private CommonEntityData createNewEntityData(final long identifier,
TAG_MAP_TO_TAG_COLLECTION_CONVERTER.convert(tags));
return data;
}

/**
* Each Atlas entity will have a base set of tags added by Atlas generation (see
* {@link AtlasTag#TAGS_FROM_OSM}). Each entity can also have additional synthetic tags (see
* {@link AtlasTag#TAGS_FROM_ATLAS}). Because an OSM {@link Node} becomes an Atlas {@link Point}
* only when it has tags, the logic here needs to make sure not to count the synthetic tags to
* make that decision. This method will calculate the total number of base + synthetic tags for
* a given {@link Node} and return {@code true} if the {@link Node} contains other, non-base and
* non-synthetic, tags.
*
* @param node
* The {@link Node} to use
* @return {@code true} if the {@link Node} contains explicit OSM tagging
*/
private boolean nodeHasExplicitOsmTags(final Node node)
{
final int nodeTagSize = node.getTags().size();
final int osmAndAtlasTagCount;

if (nodeTagSize > AtlasTag.TAGS_FROM_OSM.size())
{
int counter = 0;
for (final Tag tag : node.getTags())
{
// Tags from Atlas are the tags that only some nodes will have
if (AtlasTag.TAGS_FROM_ATLAS.contains(tag.getKey()))
{
counter++;
}
}
osmAndAtlasTagCount = AtlasTag.TAGS_FROM_OSM.size() + counter;
}
else if (nodeTagSize < AtlasTag.TAGS_FROM_OSM.size())
{
logger.error(
"Osm Node {} has {} tags, which is less than the minimum required number of tags {}",
node.getId(), nodeTagSize, AtlasTag.TAGS_FROM_OSM.size());
osmAndAtlasTagCount = nodeTagSize;
}
else
{
osmAndAtlasTagCount = AtlasTag.TAGS_FROM_OSM.size();
}

return nodeTagSize > osmAndAtlasTagCount;
}
}
2 changes: 0 additions & 2 deletions ...java/org/openstreetmap/atlas/geography/atlas/raw/slicing/changeset/RelationChangeSet.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
* {@link TemporaryEntity} objects to keep track of the bare minimum needed to create Atlas
* entities. This change set definition assumes that all individual ways and points have already
* been sliced. The following cases are handled.
* <p>
* <ul>
* <li>1. Point creation - if a new Line needs to be created, we need to create new shape points
* <li>2. Line creation - new line can be created when multipolygons are sliced by a country
Expand All @@ -37,7 +36,6 @@
* <li>8. Line deletion - in the rare case, we might delete a line that was added during country
* slicing of ways and is no longer needed
* </ul>
* <p>
*
* @author mgostintsev
*/
Expand Down
168 changes: 168 additions & 0 deletions src/test/java/org/openstreetmap/atlas/geography/atlas/pbf/OsmPbfNodeToAtlasItemTest.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
package org.openstreetmap.atlas.geography.atlas.pbf;

import static org.junit.Assert.assertEquals;

import org.junit.Rule;
import org.junit.Test;
import org.openstreetmap.atlas.geography.Latitude;
import org.openstreetmap.atlas.geography.Location;
import org.openstreetmap.atlas.geography.Longitude;
import org.openstreetmap.atlas.geography.atlas.Atlas;
import org.openstreetmap.atlas.geography.atlas.items.Node;
import org.openstreetmap.atlas.geography.atlas.items.Point;
import org.openstreetmap.atlas.utilities.collections.Iterables;

/**
* Tests OSM Node to Atlas {@link Node} and {@link Point} conversion given various use cases.
*
* @author mgostintsev
*/
public class OsmPbfNodeToAtlasItemTest
{
private static final Location OSM_NODE_LOCATION = new Location(Latitude.degrees(7.014003),
Longitude.degrees(-10.5466545));

@Rule
public final OsmPbfNodeToAtlasItemTestRule setup = new OsmPbfNodeToAtlasItemTestRule();

@Test
public void testOsmNodeNotInRelationNoTagsAtIntersection()
{
// Two OSM Ways intersecting at an OSM Node, that has no tags. We expect two Atlas Nodes
// created at the end points of each way and an Atlas Node at the intersection.
final Atlas atlas = this.setup.getNoRelationNoTagsAtIntersectionAtlas();
final Iterable<Node> nodes = atlas.nodesAt(OSM_NODE_LOCATION);
final Iterable<Point> points = atlas.pointsAt(OSM_NODE_LOCATION);

// Verify a Node was created at the middle location
assertEquals(1, Iterables.size(nodes));
assertEquals(0, Iterables.size(points));

// Verify nodes created at both end points and middle
assertEquals(5, Iterables.size(atlas.nodes()));
}

@Test
public void testOsmNodeNotInRelationNoTagsNotAtIntersection()
{
// Single OSM Way with an OSM Node in the middle without tags. We expect two Atlas Nodes
// created at the end points with a shape point in the middle.
final Atlas atlas = this.setup.getNoRelationNoTagsNoIntersectionAtlas();
final Iterable<Node> nodes = atlas.nodesAt(OSM_NODE_LOCATION);
final Iterable<Point> points = atlas.pointsAt(OSM_NODE_LOCATION);

// Verify nothing was created at the middle location
assertEquals(0, Iterables.size(nodes));
assertEquals(0, Iterables.size(points));

// Verify nodes created at end points
assertEquals(2, Iterables.size(atlas.nodes()));
}

@Test
public void testOsmNodeNotInRelationWithTagsAtIntersection()
{
// Two OSM Ways intersecting at an OSM node, that has tags. We expect two Atlas Nodes
// created at the end points of each line and both an Atlas Node and an Atlas Point at the
// intersection.
final Atlas atlas = this.setup.getNoRelationWithTagsAtIntersectionAtlas();
final Iterable<Node> nodes = atlas.nodesAt(OSM_NODE_LOCATION);
final Iterable<Point> points = atlas.pointsAt(OSM_NODE_LOCATION);

// Verify a Node was created at the middle location
assertEquals(1, Iterables.size(nodes));
assertEquals(1, Iterables.size(points));

// Verify nodes created at both end points and middle
assertEquals(5, Iterables.size(atlas.nodes()));
}

@Test
public void testOsmNodeNotInRelationWithTagsNotAtIntersection()
{
// Single OSM Way with an OSM Node in the middle with tags. We expect two Atlas Nodes
// created at the end points and an Atlas Point in the middle.
final Atlas atlas = this.setup.getNoRelationWithTagsNoIntersectionAtlas();
final Iterable<Node> nodes = atlas.nodesAt(OSM_NODE_LOCATION);
final Iterable<Point> points = atlas.pointsAt(OSM_NODE_LOCATION);

// Verify a Point was created at the middle location
assertEquals(0, Iterables.size(nodes));
assertEquals(1, Iterables.size(points));

// Verify nodes created at end points
assertEquals(2, Iterables.size(atlas.nodes()));
}

@Test
public void testOsmNodePartOfRelationNoTagsAtIntersection()
{
// Two OSM Ways meeting at an OSM node with no tags. The two ways are part of a no u-turn
// restriction with the OSM Node as the Via role. We expect two Atlas Nodes
// created at the end points and a Node in the middle.
final Atlas atlas = this.setup.getPartOfRelationNoTagsAtIntersectionAtlas();
final Iterable<Node> nodes = atlas.nodesAt(OSM_NODE_LOCATION);
final Iterable<Point> points = atlas.pointsAt(OSM_NODE_LOCATION);

// Verify a Node was created at the middle location
assertEquals(1, Iterables.size(nodes));
assertEquals(0, Iterables.size(points));

// Verify nodes created at end points and middle
assertEquals(3, Iterables.size(atlas.nodes()));
}

@Test
public void testOsmNodePartOfRelationNoTagsNotAtIntersection()
{
// Three OSM Nodes are part of a Relation. Two of them have tagging, and the middle one
// doesn't. We expect all 3 to show up as Atlas Points.
final Atlas atlas = this.setup.getPartOfRelationNoTagsNoIntersectionAtlas();
final Iterable<Node> nodes = atlas.nodesAt(OSM_NODE_LOCATION);
final Iterable<Point> points = atlas.pointsAt(OSM_NODE_LOCATION);

// Verify a single Point was created at the untagged location
assertEquals(0, Iterables.size(nodes));
assertEquals(1, Iterables.size(points));

// Verify only Points were created
assertEquals(3, Iterables.size(atlas.points()));
assertEquals(0, Iterables.size(atlas.nodes()));
}

@Test
public void testOsmNodePartOfRelationWithTagsAtIntersection()
{
// Two OSM Ways meeting at an OSM node with barrier tags. The two ways are part of a no
// u-turn restriction with the OSM Node as the Via role. We expect two Atlas Nodes
// created at the end points and an Atlas Node in the middle.
final Atlas atlas = this.setup.getPartOfRelationWithTagsAtIntersectionAtlas();
final Iterable<Node> nodes = atlas.nodesAt(OSM_NODE_LOCATION);
final Iterable<Point> points = atlas.pointsAt(OSM_NODE_LOCATION);

// Verify a Node was created at the middle location
assertEquals(1, Iterables.size(nodes));
assertEquals(1, Iterables.size(points));

// Verify nodes created at end points and middle
assertEquals(3, Iterables.size(atlas.nodes()));
}

@Test
public void testOsmNodePartOfRelationWithTagsNotAtIntersection()
{
// A single OSM Way and a single OSM Node (with tags), with a relation associating the Node
// as a house to the Way as the Street. We expect two Atlas Nodes created at the end points
// of the Way and an Atlas Point to represent the house.
final Atlas atlas = this.setup.getPartOfRelationWithTagsNoIntersectionAtlas();
final Iterable<Node> nodes = atlas.nodesAt(OSM_NODE_LOCATION);
final Iterable<Point> points = atlas.pointsAt(OSM_NODE_LOCATION);

// Verify a Point was created at the house location
assertEquals(0, Iterables.size(nodes));
assertEquals(1, Iterables.size(points));

// Verify nodes created at end points and middle
assertEquals(2, Iterables.size(atlas.nodes()));
}
}
79 changes: 79 additions & 0 deletions src/test/java/org/openstreetmap/atlas/geography/atlas/pbf/OsmPbfNodeToAtlasItemTestRule.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
package org.openstreetmap.atlas.geography.atlas.pbf;

import org.openstreetmap.atlas.geography.atlas.Atlas;
import org.openstreetmap.atlas.utilities.testing.CoreTestRule;
import org.openstreetmap.atlas.utilities.testing.TestAtlas;

/**
* {@link OsmPbfNodeToAtlasItemTest} test cases. This class relies on "loadFromJosmOsmResource"
* methods, instead of text atlases, because it relies on loading from PBF and testing the
* conversion from OSM PBF to Atlas Item. Loading in any other format, would skip this step.
*
* @author mgostintsev
*/
public class OsmPbfNodeToAtlasItemTestRule extends CoreTestRule
{
@TestAtlas(loadFromJosmOsmResource = "noRelationNoTagsNoIntersectionAtlas.osm")
private Atlas atlasWithoutRelationNoTagsNoIntersection;

@TestAtlas(loadFromJosmOsmResource = "partOfRelationNoTagsAtIntersectionAtlas.osm")
private Atlas atlasPartOfRelationNoTagsAtIntersection;

@TestAtlas(loadFromJosmOsmResource = "partOfRelationWithTagsNoIntersectionAtlas.osm")
private Atlas atlasPartOfRelationWithTagsNoIntersection;

@TestAtlas(loadFromJosmOsmResource = "partOfRelationWithTagsAtIntersectionAtlas.osm")
private Atlas atlasPartOfRelationWithTagsAtIntersection;

@TestAtlas(loadFromJosmOsmResource = "noRelationWithTagsNoIntersectionAtlas.osm")
private Atlas atlasWithoutRelationWithTagsNoIntersection;

@TestAtlas(loadFromJosmOsmResource = "noRelationNoTagsAtIntersectionAtlas.osm")
private Atlas atlasWithoutRelationNoTagsAtIntersection;

@TestAtlas(loadFromJosmOsmResource = "noRelationWithTagsAtIntersectionAtlas.osm")
private Atlas atlasWithoutRelationWithTagsAtIntersection;

@TestAtlas(loadFromJosmOsmResource = "partOfRelationNoTagsNoIntersectionAtlas.osm")
private Atlas atlasPartOfRelationNoTagsNoIntersection;

public Atlas getNoRelationNoTagsAtIntersectionAtlas()
{
return this.atlasWithoutRelationNoTagsAtIntersection;
}

public Atlas getNoRelationNoTagsNoIntersectionAtlas()
{
return this.atlasWithoutRelationNoTagsNoIntersection;
}

public Atlas getNoRelationWithTagsAtIntersectionAtlas()
{
return this.atlasWithoutRelationWithTagsAtIntersection;
}

public Atlas getNoRelationWithTagsNoIntersectionAtlas()
{
return this.atlasWithoutRelationWithTagsNoIntersection;
}

public Atlas getPartOfRelationNoTagsAtIntersectionAtlas()
{
return this.atlasPartOfRelationNoTagsAtIntersection;
}

public Atlas getPartOfRelationNoTagsNoIntersectionAtlas()
{
return this.atlasPartOfRelationNoTagsNoIntersection;
}

public Atlas getPartOfRelationWithTagsAtIntersectionAtlas()
{
return this.atlasPartOfRelationWithTagsAtIntersection;
}

public Atlas getPartOfRelationWithTagsNoIntersectionAtlas()
{
return this.atlasPartOfRelationWithTagsNoIntersection;
}
}
22 changes: 22 additions & 0 deletions ...urces/org/openstreetmap/atlas/geography/atlas/pbf/noRelationNoTagsAtIntersectionAtlas.osm
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<?xml version='1.0' encoding='UTF-8'?>
<osm version='0.6' generator='JOSM'>
<node id='-1409998' action='modify' lat='7.01378166705' lon='-10.54718467843' />
<node id='-1409999' action='modify' lat='7.01400307686' lon='-10.54665453727' />
<node id='-1410001' action='modify' lat='7.01357328126' lon='-10.54613226949' />
<node id='-1410003' action='modify' lat='7.01317734799' lon='-10.54639734007' />
<node id='-1410005' action='modify' lat='7.01327244286' lon='-10.54497940386' />
<way id='-1410000' action='modify'>
<nd ref='-1409998' />
<nd ref='-1409999' />
<nd ref='-1410001' />
<tag k='highway' v='motorway' />
<tag k='oneway' v='yes' />
</way>
<way id='-1410008' action='modify'>
<nd ref='-1410003' />
<nd ref='-1409999' />
<nd ref='-1410005' />
<tag k='highway' v='primary' />
<tag k='oneway' v='yes' />
</way>
</osm>
Loading

0 comments on commit 386cdb2

Please sign in to comment.