SubAtlas cut type functionality #276

MikeGost · 2018-11-20T19:58:58Z

Description:

The original intent was to add more granularity to the type of cutting performed when creating a subAtlas. Specifically, the intent was to keep the existing soft-cut option and add two new "hard-cut" and "hard-cut-relations-only" options. Both of these have been added for the subAtlas with predicate call and their examples and logic is documented in the code.
Updated the Polygon sub-atlas functionality to follow the same model as the Predicate sub-atlas call.
Refactored all sub-atlas creation out of BareAtlas and into SubAtlasCreator, for cleaner behavior separation.
Several new Atlas APIs checking for full feature containment within a given Polygon.
Minor optimizations surrounding relation handling, leveraging relationsLowerOrderFirst method to remove the looping in the existing subAtlas code.
Found an bug in the Rectangle.forCorners method. The method takes in a lower left and an upper right location. However, if we pass in an upper left and lower right, the Rectangle creation will succeed, however, successive fullyGeometricallyEncloses method calls will fail because they will use member variable fields for what they think is the lower left and upper right and compare the latitude and longitude of those locations to some target location, leading to an invalid result. I've added constraints to verify that the lower left and upper right are actually that.

DNM: There is more refactor possible. Possible improvements include:

~~More documentation~~
~~Apply the same logic to the subAtlas with Polygon method~~
~~Refactor to move all subAtlas code away from BareAtlas - that class has grown too large~~

Potential Impact:

None - subAtlas logic did not change. There are now more Atlas APIs available and a finer granularity for all users to specify a cut type when creating sub atlases.

Unit Test Approach:

All existing unit tests pass. I've created new unit tests to capture all entity within behavior, new subAtlas behavior and to verify the Rectangle forCorners fix.

Test Results:

All integration tests pass.

In doubt: Contributing Guidelines

… deprecating it

…on to call out awt insideness definition

…for all

lucaspcram

These changes look good, still digesting a lot of it. Good tests!

lucaspcram · 2018-11-25T05:15:29Z

src/main/java/org/openstreetmap/atlas/geography/Rectangle.java

@@ -60,6 +60,15 @@ public static Rectangle forCorners(final Location lowerLeft, final Location uppe
        {
            throw new CoreException("Cannot build a Rectangle with one of the corners being null.");
        }
+        // Sanity check to avoid invalid Rectangles
+        else if (lowerLeft.isNorthOf(upperRight))


Good catch. This is a bug in pyatlas as well.

lucaspcram · 2018-11-25T05:19:47Z

src/main/java/org/openstreetmap/atlas/geography/atlas/AtlasCutType.java

+ *
+ * @author mgostintsev
+ */
+public enum AtlasCutType


jklamer · 2018-11-25T23:01:01Z

src/main/java/org/openstreetmap/atlas/geography/atlas/Atlas.java

     */
-    Optional<Atlas> subAtlas(Predicate<AtlasEntity> matcher);
+    Optional<Atlas> subAtlas(Predicate<AtlasEntity> matcher, AtlasCutType cutType);


Please leave the backwards compatible function in the interface.

Any reason why? The function is still available for use in the BareAtlas, marked as deprecated. So it will be backward compatible until all downstream code is migrated.

Any algorithms operating on the Atlas interface won't be able to use the old function without casting or converting to a specific implementation.

I'll leave it open for discussion, could go either why on this. Updating the API as a breaking change with the new signature makes sense to me, but if this causes too much churn, we can leave it as is, just not as clean, having two subAtlas methods here, with one doing the same thing as the other.

I believe you can make a default implementation of the original signature that uses the newer signature. Then with you put a @depricated on the old signature, and remove at the next major version increment. That would be my preference. Gives all the users a longer time to adjust for the same result.

Good idea, I'll give that a shot.

MikeGost · 2018-11-25T23:23:12Z

More changes incoming.

adahn6 · 2018-11-26T17:57:37Z

src/test/java/org/openstreetmap/atlas/geography/RectangleTest.java

+        Assert.assertTrue("Rectangle 3 fully contains rectangle 2",
+                this.rectangle3.fullyGeometricallyEncloses(this.rectangle2));
+        Assert.assertTrue("That means, it should also cover it partially",
+                this.rectangle3.overlaps(this.rectangle4));


is this supposed to be

this.rectangle3.overlaps(this.rectangle2)

?

Good catch! This code was re-ordered, hadn't noticed this. Fixed!

matthieun · 2018-11-26T19:11:41Z

src/main/java/org/openstreetmap/atlas/geography/atlas/AbstractAtlas.java

+    public Iterable<Area> areasWithin(final Polygon polygon)
+    {
+        final Iterable<Area> areas = this.getAreaSpatialIndex().get(polygon.bounds());
+        return Iterables.filter(areas, area ->


Optional: This is the old way of filtering. How about bringing new changes with Iterables.stream(...).filter(...)?

Saw this too, wanted to stay consistent to the rest of the class. Let me go ahead and re-write the rest of the function to follow the same format.

…olygons and tests

…s predicates

AlexHsieh22

Nice work! A couple questions:

Would it ever be possible to have subAtlas(Polygon) call subAtlas(matcher) underneath? It feels like the should really do the same thing, but subAtlas(Polygon) is a convenience method.
Does it make sense to have a subAtlas base class or interface? If 1 above can be done, there can be a default implementation for subAtlas(Polygon) and there could be a centralized source for logging and the packaging of the packed atlas.

MikeGost · 2018-11-27T01:26:29Z

@AlexHsieh22 - great ideas!

I believe we chatted about this briefly last week - I totally agree, this would make everything a lot cleaner.
Good idea! This can be added.

I'll create an issue to track these ideas and get them implemented!

AlexHsieh22

Thanks for filing the Github issue @MikeGost !

MikeGost added 9 commits November 19, 2018 11:25

subAtlas predicate with cut type working and tested

f8a31d7

Removing default soft-cut sub-atlas with predicate from interface and…

09900e0

… deprecating it

Adding containment within polygon atlas APIs for lines, edges and areas

0a1fc01

Fixing rectangle bug, updating tests

c2883b0

Spotless

dd66f30

Fixing DynamicAtlas

252c55e

removing comments

aaffa3e

Adding Line, Edge and LineItem tests

6fa75bc

Adding Area and AtlasItem tests for containment, updating documentati…

014eddd

…on to call out awt insideness definition

MikeGost added Bug Enhancement New Feature labels Nov 20, 2018

MikeGost requested review from adahn6, matthieun, lucaspcram and jwpgage November 20, 2018 19:58

MikeGost changed the title ~~SubAtlas cut type functionality~~ DNM: SubAtlas cut type functionality Nov 20, 2018

Relation items within polygon, hard cut with polygon subatlas, tests …

bb8368a

…for all

lucaspcram reviewed Nov 25, 2018

View reviewed changes

jklamer suggested changes Nov 25, 2018

View reviewed changes

adahn6 reviewed Nov 26, 2018

View reviewed changes

matthieun reviewed Nov 26, 2018

View reviewed changes

MikeGost added 5 commits November 26, 2018 12:15

Adding subAtlas back to Atlas interface, finishing hard cutting for P…

9e1db07

…olygons and tests

Updating to use easier to understand stream/filter logic for all atla…

72b18f2

…s predicates

Fixing rectangle test to match desired test

acafb97

Adding Relation logger back

38e0724

Resolving merge conflict from upstream

3b56c7d

MikeGost force-pushed the hardCut branch from b5b6978 to 3b56c7d Compare November 26, 2018 21:36

Moving sub atlas cutting to separate class

a868539

MikeGost added 2 commits November 26, 2018 15:02

Fixing logger

e009dad

Sonarcube fix

2b36a29

adahn6 previously approved these changes Nov 26, 2018

View reviewed changes

Cleaning up cutting logging

0af3126

MikeGost dismissed adahn6’s stale review via 0af3126 November 26, 2018 23:11

AlexHsieh22 reviewed Nov 27, 2018

View reviewed changes

MikeGost mentioned this pull request Nov 27, 2018

Refactor Subatlas functionality #278

Open

AlexHsieh22 previously approved these changes Nov 27, 2018

View reviewed changes

Cleanup and setting the stage for subAtlas interface

7543c75

MikeGost dismissed AlexHsieh22’s stale review via 7543c75 November 27, 2018 20:22

MikeGost force-pushed the hardCut branch from e63a062 to 7543c75 Compare November 27, 2018 20:22

MikeGost changed the title ~~DNM: SubAtlas cut type functionality~~ SubAtlas cut type functionality Nov 27, 2018

Merge branch 'dev' into hardCut

f998723

lucaspcram approved these changes Nov 27, 2018

View reviewed changes

matthieun merged commit 4066587 into osmlab:dev Nov 28, 2018

matthieun added this to the 5.2.9 milestone Dec 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SubAtlas cut type functionality #276

SubAtlas cut type functionality #276

MikeGost commented Nov 20, 2018 •

edited

Loading

lucaspcram left a comment

lucaspcram Nov 25, 2018

lucaspcram Nov 25, 2018

jklamer Nov 25, 2018

MikeGost Nov 25, 2018

jklamer Nov 25, 2018

MikeGost Nov 25, 2018

jklamer Nov 25, 2018

MikeGost Nov 25, 2018

MikeGost commented Nov 25, 2018

adahn6 Nov 26, 2018

MikeGost Nov 26, 2018

matthieun Nov 26, 2018

MikeGost Nov 26, 2018

AlexHsieh22 left a comment

MikeGost commented Nov 27, 2018

AlexHsieh22 left a comment

SubAtlas cut type functionality #276

SubAtlas cut type functionality #276

Conversation

MikeGost commented Nov 20, 2018 • edited Loading

Description:

Potential Impact:

Unit Test Approach:

Test Results:

lucaspcram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeGost commented Nov 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexHsieh22 left a comment

Choose a reason for hiding this comment

MikeGost commented Nov 27, 2018

AlexHsieh22 left a comment

Choose a reason for hiding this comment

MikeGost commented Nov 20, 2018 •

edited

Loading