Repository navigation
Conversation
Signed-off-by: Mike Barry <msb5014@gmail.com>
msbarry
commented
Oct 29, 2026
|
|
||
| private TopologyValidationError validateIndexedShell(String wkt) { | ||
| IsValidOp validOp = new IsValidOp(read(wkt)); | ||
| validOp.setMinHolesIndexedShell(0); |
Contributor
Author
There was a problem hiding this comment.
I added this to keep the tests verifying index behavior smaller, if you want I could set the min holes to 0 on all tests in this file so they all exercise the new code path, or make the indexed ones create 20 extra holes to force it without the setMinHoles method?
Contributor
Author
There was a problem hiding this comment.
Also changed this to validate all test cases with and without an index.
msbarry
commented
Oct 29, 2026
| * Tests performance of IsValidOp.checkHolesInShell | ||
| * for polygons with a large shell and many holes. | ||
| */ | ||
| public class IsValidHolesInShellPerfTest extends PerformanceTestCase { |
Contributor
Author
There was a problem hiding this comment.
On my m1 macbook:
| Before | After |
|
|
msbarry
commented
Oct 29, 2026
| public void testPolygonHoleOutsideShellInConcavityTouch() { | ||
| checkInvalidIndexedShell(TopologyValidationError.HOLE_OUTSIDE_SHELL, | ||
| "POLYGON ((10 10, 10 90, 40 90, 40 30, 60 30, 60 90, 90 90, 90 10, 10 10), (15 70, 25 70, 25 80, 15 80, 15 70), (70 50, 80 50, 80 60, 70 60, 70 50), (40 50, 50 45, 50 55, 40 50))"); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
These test add some extra coverage around the indexed version:
Contributor
Author
|
Hey @dr-jts - just checking on this, does this change look reasonable? Is there any other testing you want me to do or changes to make on this one? |
msbarry
force-pushed
the
isvalid-shell-index
branch
from
November 7, 2026 22:34
43e30f3 to
9ef628e
Compare
Signed-off-by: Mike Barry <msb5014@gmail.com>
msbarry
force-pushed
the
isvalid-shell-index
branch
from
November 7, 2026 22:42
9ef628e to
5847d01
Compare
Contributor
Author
|
I'm not quite sure what's going on with the ECA check, I've got a valid ECA on file and signed the commit that it's complaining about 5847d01 🤔 |
Signed-off-by: Mike Barry <msb5014@gmail.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While profiling planetiler I noticed
IsValidOp.checkHolesInShellcan get slow for very complex polygons with lots of holes since it's testing whether each hole is inside the shell without an index. For example Lake Nasser which has 1.3M points and thousands of island holes takes 14.7 seconds to run an isValid() check.This PR uses
IndexedPointInAreaLocatorto test if holes are inside the shell. I added a perf test which showed that the overhead of building the index was larger than the perf gain for less than 20 holes so I added a threshold to only build the index when there are >20 holes. This brings Lake Nasser validity check from 14.7 seconds down to 1.6 seconds.I used opus 5.5 to help identify the bottleneck and prototype the improvement.