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

Add support to rotate rings to avoid split #180

Merged
merged 13 commits into from
Sep 1, 2022
Merged

Add support to rotate rings to avoid split #180

merged 13 commits into from
Sep 1, 2022

Conversation

theroggy
Copy link
Contributor

@theroggy theroggy commented Aug 30, 2022

Remark: depends on #179 to be merged first!

Support rotating rings in cut to avoid unneeded splits happening.

I noticed that there is some logic in dedup that merges adjacent lines:

  • I'm not sure if this code is meant to do something similar but there are at least cases where it doesn't.
  • Performance-wise, rotating the rings is very efficiënt while mergine lines using shapely is rather "costly". I noticed a performance gain of 19.3 s to 18.1 s for my test file if the ring rotation is activated, so I guess (not sure) it might have to do with the linemerge logic having less work.

references #178

@mattijn
Copy link
Owner

mattijn commented Aug 31, 2022

First, thanks for this PR!

I like it how you rotate to the first junction part in the fast_split function.

I can imagine this reduce the workload in the Dedup class where it tries to merge arcs when possible.

As mentioned in the inline comme t, maybe we can track the type directly during extraction?

Also would be great if this PR only reflect the changes regarding the rotation.

@theroggy
Copy link
Contributor Author

theroggy commented Aug 31, 2022

As mentioned in the inline comme t, maybe we can track the type directly during extraction?

It's possible, but I don't think there is an advantage in denormalizing this information in "permanent" structures as we only need access to it via linestring index in this specific code path? I might be wrong, but I also doubt it would give a measurable performance benefit?

Also would be great if this PR only reflect the changes regarding the rotation.

I made the changes independent of possible changes to the join logic.

@mattijn mattijn merged commit d310033 into mattijn:master Sep 1, 2022
@mattijn
Copy link
Owner

mattijn commented Sep 1, 2022

Thanks for making this PR more compact. All tests pass. Thanks for pushing!

@theroggy theroggy deleted the Add-support-to-rotate-rings-to-avoid-split branch September 1, 2022 19:28
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