[WIP patch] Improve PowerLines test performance

[gaben]

I have to check why (what and why I missed previously), but this single addition dramatically improves the test execution time. The test was extended in r18553.

src/org/openstreetmap/josm/data/validation/tests/PowerLines.java

@@ -628 +628 @@
      * @return {@code true} if primitive has a tag that marks it as a water area or boundary of a water area
      */
     private static boolean concernsWaterArea(OsmPrimitive p) {
-        return p.hasTag("water", "river", "lake") || p.hasKey("waterway") || p.hasTag("natural", "coastline");
+        return (p.hasTag("water", "river", "lake") || p.hasKey("waterway") || p.hasTag("natural", "coastline")) && p.concernsArea();
     }
 
     /**

(Currently migrating to a new dev environment so publishing even the WIP patches.)

[taylor.smock]

In 18838/josm:

Fix #23008: Improve PowerLines test performance (patch by gaben)

[GerdP]

I wonder in what case ways with natural=coastline will be used after this patch.

[taylor.smock]

natural=coastline

This sounds like I need to write a test case. :)

[gaben]

Yes, maybe it was a hasty change. I marked it WIP for this reason.

[taylor.smock]

I did test it in my standard test area (Mesa County, CO) which has no coastlines...

Anyway, this is why I like tests. It keeps me from borking stuff accidentally.

I'll double check, but when I was looking at this, I did check what concernsArea does. Which is comparing tags against a set of known area tags, and (if it is a relation) check to see if the relation is a multipolygon.

EDIT: To clarify, I did think about just using the relation multipolygon check instead of concernsArea, but I though concernsArea was more readable.

[GerdP]

You'll probably never see a complete area built with natural=coastline. OTOH we may exclude small waterways like ditch, drain, stream as these are probably never a reason for a longer distance between two power towers?

[taylor.smock]

Add ocean tests

[taylor.smock]

Oddly enough, the concernsArea call seems to have no effect on the tests I wrote. Can someone check them for sanity?

[GerdP]

test data that should show no warnings from powerline test

[GerdP]

Try with the attached sample. When you remove the coastline way the powerline test complains. Tested with r18822

[taylor.smock]

In 18840/josm:

Revert r18838, see #23008: natural=coastline does not work well with the changes

This adds a test to ensure future changes don't break the natural=coastline
branch.

[gaben]

I have to test, but the code with concernsArea() is still better in my opinion. Can someone estimate how many cases could be where the power line crosses coastlines like the example data?

Probably there aren't much, and until then the performance improvement is a welcomed change.

[taylor.smock]

I'd guess that most of the speed improvement could also be done with primitive instanceof Relation && ((Relation) primitive).isMultipolygon().

[GerdP]

Reg. coastline: I gave an example here https://josm.openstreetmap.de/ticket/20716#comment:28
I guess it is a rare problem, but coastline ways should not have a big impact on performance. Small waterways are much more likely to appear in data and are also likely to "cross" power lines.
A possible performance improvement might be to perform the crossing ways test only when missing power nodes where found. The current implementation collects and indexes all waterway objects even if no powerlines exist in the data, right?

[taylor.smock]

The current implementation collects and indexes all waterway objects even if no powerlines exist in the data

It should, yes. It would be much more expensive to go back and revisit all ways after finding a powerline.

With that said, we could try adding a new collection (e.g. foundWaterways) and we run the indexing in endTest if, and only if, we need it for powerlines.

[gaben]

Note to myself: maybe I could utilize QuadBuckets somewhere in this test to gain some performance, possibly replace the existing "line hanging over a water area" part.