AtlasDelta and AtlasDeltaGenerator Revamp #195

hallahan · 2018-08-14T21:27:45Z

Description:

This PR re-implements AtlasDeltaGenerator, as the old implementation was not working. Now, this tool will generate GeoJSON both for the features as well as the relations. It will also output the text-based "diff view friendly string".

Potential Impact:

AtlasDelta is mainly used for testing and data verification.

Unit Test Approach:

Because this PR is focused on the GeoJSON generation, I have AtlasDeltaGeoJsonIntegrationTest generate GeoJSON and assert certain things about it such as:

length of serialized JSON
feature count
if the features have the necessary diff related properties

Test Results:

In addition, I have been running AtlasDeltaGenerator on various atlases (just shards), and then take the generated GeoJSON and look at it in JOSM.

Note that I'm continuing with the existing approach where we separate relations from nodes and ways in the delta GeoJSONs. This helps reduce noise, because typically when a relation is modified, a lot of underlying geometry is affected.

…that include before and after entities. Also, the generator CLI is not working.

hallahan · 2018-08-15T01:02:34Z

Improved docs for making shaded JAR and setting up in IntelliJ IDEA.

lucaspcram · 2018-08-15T16:45:16Z

CONTRIBUTING.md

+
+``` 
+java -cp atlas-5.1.8-SNAPSHOT-shaded.jar org.openstreetmap.atlas.geography.atlas.delta.AtlasDeltaGenerator <args...>
+```


Great README addition.

jwpgage · 2018-08-15T18:16:34Z

src/main/java/org/openstreetmap/atlas/geography/atlas/delta/Diff.java

+        if (afterEntity != null)
+        {
+            final Map<String, String> afterTags = afterEntity.getTags();
+            afterTags.put("diff", "AFTER");


It seems like this is the only difference between these code blocks. Maybe could move the if(null) check here and avoid duplicating a lot of this code?

Great point, I will change this.

jwpgage · 2018-08-15T18:17:28Z

src/main/java/org/openstreetmap/atlas/geography/atlas/delta/AtlasDelta.java

@@ -36,28 +36,29 @@
 * Difference between two {@link Atlas}.
 *
 * @author matthieun
+ * @author hallahan


Not sure a variable name change warrants an author tag ;)

LOL, yeah, you're right! I'll take it out. I didn't realize I did it for that file.

lucaspcram · 2018-08-15T18:37:59Z

...est/java/org/openstreetmap/atlas/geography/atlas/delta/AtlasDeltaGeoJsonIntegrationTest.java

+     */
+    @Test
+    public void parseGeoJson()
+    {


This functionality is going to be a huge win.

lucaspcram · 2018-08-15T18:38:47Z

src/main/java/org/openstreetmap/atlas/geography/atlas/delta/AtlasDeltaGenerator.java

+
+        final AtlasDelta delta = new AtlasDelta(beforeAtlas, afterAtlas).generate();
+
+        final String txt = delta.toDiffViewFriendlyString();


Nit: we try to avoid any and all abbreviations in atlas. We would prefer text over txt. If I didn't say it I know @MikeGost would have :)

Sounds good, I'll change that!

lucaspcram · 2018-08-15T18:39:07Z

src/main/java/org/openstreetmap/atlas/geography/atlas/delta/AtlasDeltaGenerator.java

+        final AtlasDelta delta = new AtlasDelta(beforeAtlas, afterAtlas).generate();
+
+        final String txt = delta.toDiffViewFriendlyString();
+        final File txtFile = new File(


Same thing here.

lucaspcram · 2018-08-15T18:40:53Z

src/main/java/org/openstreetmap/atlas/geography/atlas/packed/README.md

@@ -1,6 +1,6 @@
 # `PackedAtlas`

-This is the base `Atlas`. It is immutable, and stores all its items in large arrays in memory.
+This is the before `Atlas`. It is immutable, and stores all its items in large arrays in memory.


I like the new before/after naming choices over base/alter.

Cool, I was hoping it was ok.

lucaspcram

I really like these changes. Just see my minor nit about a variable name :)

jwpgage · 2018-08-15T21:12:46Z

src/main/java/org/openstreetmap/atlas/geography/atlas/delta/AtlasDeltaGenerator.java

-    private static final Switch<File> OUTPUT_FOLDER_SWITCH = new Switch<>("outputFolder",
-            "The path of the output folder", string -> new File(string), Optionality.REQUIRED);
+    private static final Switch<Path> OUTPUT_DIR_SWITCH = new Switch<>("outputDir",
+            "The path of the output directory.", Paths::get, Optionality.REQUIRED);


Nit: outputDir -> outputDirectory. As @lucaspcram said we try to avoid abbreviations, even popular ones

hallahan · 2018-08-15T22:30:46Z

Ok @lucaspcram and @jwpgage , ready for another review!

MikeGost

Nice changes @hallahan! Couple minor nits regarding abbreviations.

MikeGost · 2018-08-15T23:44:22Z

...est/java/org/openstreetmap/atlas/geography/atlas/delta/AtlasDeltaGeoJsonIntegrationTest.java

+    public void testGeoJson()
+    {
+        final String geoJson = delta.toGeoJson();
+        final int len = geoJson.length();


Nit: len --> length

MikeGost · 2018-08-15T23:44:42Z

...est/java/org/openstreetmap/atlas/geography/atlas/delta/AtlasDeltaGeoJsonIntegrationTest.java

+    public void testRelationsGeoJson()
+    {
+        final String relationsGeoJson = delta.toRelationsGeoJson();
+        final int len = relationsGeoJson.length();


hallahan · 2018-08-16T01:05:09Z

Ok, @MikeGost , ready for review / merge? Fixed len abbrev.

MikeGost · 2018-08-16T17:57:25Z

Looks good! I'm comfortable with the merge, wanted @matthieun to look at it first since he was original author.

lucaspcram · 2018-08-16T17:58:31Z

Ah sorry. Looks like I jumped the gun on this one.

hallahan · 2018-08-16T17:59:30Z

@MikeGost and @lucaspcram - I'll have another PR out today, and maybe @matthieun can chime in and I can make any changes needed there?

MikeGost · 2018-08-16T18:10:05Z

@hallahan Sounds good! @lucaspcram you're fine!

hallahan added 7 commits August 14, 2018 10:33

Revamp AtlasDelta and AtlasDeltaGenerator. We now get GeoJSON deltas …

f438bad

…that include before and after entities. Also, the generator CLI is not working.

./gradlew spotlessApply

15c3a76

rename base and alter to before and after

3fe513e

AtlasDelta integration testing. Also, fixed a typo bug!

ce757f4

spotless apply

4a65490

explain parse geojson test

a88b05d

Improve log4j instructions

f0ed314

lucaspcram reviewed Aug 15, 2018

View reviewed changes

MikeGost added Enhancement Documentation labels Aug 15, 2018

jwpgage reviewed Aug 15, 2018

View reviewed changes

lucaspcram reviewed Aug 15, 2018

View reviewed changes

lucaspcram requested changes Aug 15, 2018

View reviewed changes

jwpgage reviewed Aug 15, 2018

View reviewed changes

Account for PR comments.

9dfaa2f

lucaspcram previously approved these changes Aug 15, 2018

View reviewed changes

fix abbrev

6415640

hallahan dismissed lucaspcram’s stale review via 6415640 August 15, 2018 22:30

lucaspcram previously approved these changes Aug 15, 2018

View reviewed changes

MikeGost previously approved these changes Aug 15, 2018

View reviewed changes

no more abbrev.

3043dc7

hallahan dismissed stale reviews from MikeGost and lucaspcram via 3043dc7 August 16, 2018 01:04

lucaspcram approved these changes Aug 16, 2018

View reviewed changes

lucaspcram merged commit 1686af6 into osmlab:dev Aug 16, 2018

hallahan mentioned this pull request Aug 16, 2018

Atlas Delta Generator Sharded #199

Merged

matthieun added this to the 5.1.9 milestone Aug 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AtlasDelta and AtlasDeltaGenerator Revamp #195

AtlasDelta and AtlasDeltaGenerator Revamp #195

hallahan commented Aug 14, 2018

hallahan commented Aug 15, 2018

lucaspcram Aug 15, 2018

jwpgage Aug 15, 2018

hallahan Aug 15, 2018

jwpgage Aug 15, 2018

hallahan Aug 15, 2018

lucaspcram Aug 15, 2018

lucaspcram Aug 15, 2018

hallahan Aug 15, 2018

lucaspcram Aug 15, 2018

lucaspcram Aug 15, 2018

hallahan Aug 15, 2018

lucaspcram left a comment •

edited

Loading

jwpgage Aug 15, 2018

hallahan commented Aug 15, 2018

MikeGost left a comment

MikeGost Aug 15, 2018

MikeGost Aug 15, 2018

hallahan commented Aug 16, 2018

MikeGost commented Aug 16, 2018

lucaspcram commented Aug 16, 2018

hallahan commented Aug 16, 2018

MikeGost commented Aug 16, 2018


		final AtlasDelta delta = new AtlasDelta(beforeAtlas, afterAtlas).generate();

		final String txt = delta.toDiffViewFriendlyString();

AtlasDelta and AtlasDeltaGenerator Revamp #195

AtlasDelta and AtlasDeltaGenerator Revamp #195

Conversation

hallahan commented Aug 14, 2018

Description:

Potential Impact:

Unit Test Approach:

Test Results:

hallahan commented Aug 15, 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

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

lucaspcram left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hallahan commented Aug 15, 2018

MikeGost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hallahan commented Aug 16, 2018

MikeGost commented Aug 16, 2018

lucaspcram commented Aug 16, 2018

hallahan commented Aug 16, 2018

MikeGost commented Aug 16, 2018

lucaspcram left a comment •

edited

Loading