Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve performance of unary union for many short lines #213

Closed
wants to merge 7 commits into from

Conversation

dbaston
Copy link
Member

@dbaston dbaston commented Aug 27, 2019

This PR adds a benchmark for the case of unioning many line segments through unary union, and then improves the performance of that benchmark by roughly 90-95% for the case of 200 lines.

@dbaston dbaston changed the title WIP: Improve performance of unary union for many short lines Improve performance of unary union for many short lines Aug 27, 2019
@dr-jts
Copy link
Contributor

dr-jts commented Aug 27, 2019

(DB) I think the tickets referred to in the UnaryUnion? comments suggest that the cascaded union helps robustness by unioning nearby lines together (which supposedly helps because of common bit removal.)

That makes sense. And could help in some cases - but only ones where there is a fair degree of locality in the line set.

And ultimately it's not possible to improve intersection computation to the point where noding will be fully robust, because it's not possible to correctly represent all intersections of lines with DP endpoints using DP values. Hence the need for some kind of snapping approach.

@dr-jts
Copy link
Contributor

dr-jts commented Aug 27, 2019

See GEOS 962 and GEOS 982 for some background.

@dr-jts
Copy link
Contributor

dr-jts commented Aug 27, 2019

Just to be clear, am I right in thinking that this enhancement does not eliminate the possibility that Unary Union will encounter a case which has extremely slow performance? There could be a pathlogical case which contains a noding failure, drops through to Cascaded Union, and then runs very slowly.

@dbaston
Copy link
Member Author

dbaston commented Aug 27, 2019

Correct, although it still improves performance in the event that a cascaded union is run.

@dbaston
Copy link
Member Author

dbaston commented Aug 29, 2019

Failing test is issue-geos-392.xml, will have to dig into it more. Result is valid, and difference between actual and expected results is EMPTY, yet they fail an equality test. Fun!

@dbaston
Copy link
Member Author

dbaston commented Sep 5, 2019

@dr-jts I'm tempted to just change the expected result of issue-geos-392.xml. Both results are valid (in master and in this PR). This test case doesn't pass w/JTS, so I don't have a clear reason to prefer one over the other. Any thoughts?

@dr-jts
Copy link
Contributor

dr-jts commented Sep 5, 2019

@dr-jts I'm tempted to just change the expected result of issue-geos-392.xml. Both results are valid (in master and in this PR). This test case doesn't pass w/JTS, so I don't have a clear reason to prefer one over the other. Any thoughts?

Can you look at both outputs in the JTS TestBuilder and see where the differences are? The latest TestBuilder includes a Diff.diffSegments function which can help here. Also check that the noding is correct - can use the Noding.findNodePoints function to display nodes (intersections) in the linework. The output should really be fully noded.

@dbaston
Copy link
Member Author

dbaston commented Sep 6, 2019

I can't build the latest TestBuilder at the moment...I'm certain it's something on my end.

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-assembly-plugin:2.6:single (make-assembly) on project jts-tests: Execution make-assembly of goal org.apache.maven.plugins:maven-assembly-plugin:2.6:single failed: An API incompatibility was encountered while executing org.apache.maven.plugins:maven-assembly-plugin:2.6:single: java.lang.ExceptionInInitializerError: null

Noding.findNodePoints returns MULTIPOINT EMPTY for both version of the output. I'll post an XML of the two outputs for reference. A is from master and B is from this PR. (extension changed because GH "doesn't support" XML) diff.txt

@dr-jts
Copy link
Contributor

dr-jts commented Sep 16, 2019

I looked at the two geometries in the TestBuilder. They look very, very close.. Unfortunately the TestBuilder doesn't expose the equalsExact with a tolerance, so I can't test that. But I suspect that would report them as equal. Can you try that?

Improves segment unary union perf test by 45%.
Improves performance of segment unary union benchmark by 85%.
- Change two tests to use GEOSEquals instead of WKT comparison, so
as not to be sensitive to component ordering.
- Change two tests to expect that the union of LINESTRING EMPTY is
LINESTRING EMPTY, instead of GEOMETRYCOLLECTION EMPTY. This is
consistent with JTS, as with how other types are handled.
Bypassing cascaded union of lines produces a different but still-correct
result.
Doxygen is apparently unable to resolve the references to an enum class.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants