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

catch and handle topology exceptions when splitting geometries #362

Merged
merged 2 commits into from
Mar 23, 2021

Conversation

tyrasd
Copy link
Member

@tyrasd tyrasd commented Mar 22, 2021

Description

After #272 (version 0.6.0), topology exceptions thrown during the intersection calculations in the GeometrySplitter class are not (always) catched anymore. This solves the problem by adding an additional try-catch block and returning an empty geometry if a topology exception is occurring.

Checklist

@tyrasd tyrasd added the bug Something isn't working as expected label Mar 22, 2021
@tyrasd tyrasd added this to the release 0.7.0 milestone Mar 22, 2021
@tyrasd tyrasd changed the title catch and handle topology exceptions when splitting geometries 🚧 catch and handle topology exceptions when splitting geometries Mar 22, 2021
@tyrasd tyrasd changed the title 🚧 catch and handle topology exceptions when splitting geometries catch and handle topology exceptions when splitting geometries Mar 23, 2021
@tyrasd tyrasd added the waiting for review This pull request needs a code review label Mar 23, 2021
@FabiKo117
Copy link
Contributor

We used to say that with the OSHDB the user has access to erroneous OSM data as well, including features that were falsely tagged, or also had a broken geometry. How would this be affected with this fix here? Could someone still perform an analysis looking at such data that has/had some issues regarding its topology through the OSHDB?

@tyrasd
Copy link
Member Author

tyrasd commented Mar 23, 2021

We used to say that with the OSHDB the user has access to erroneous OSM data

I'm not sure where exactly and in which context we said that, but certain data problems are just too grave to be able to access via the oshdb-api – entities with no or invalid coordinates, for example. The issue with topology exceptions is that these come from an external library (JTS), which the oshdb-api needs for performing geometry operations like clipping. When JTS refuses to do that (because the OSM data is too broken), there's little alternatives at the moment for us. I once proposed to evaluate a different algorithm for clipping in #214, which might be more robust, but that's definitely out of scope of this PR.

What I think we always promised is that the oshdb-api would not crash, even when confronted with broken/invalid OSM data, see e.g. our unit test. This PR simply restores the behavior from version 0.5 again.

Could someone still perform an analysis looking at such data that has/had some issues regarding its topology through the OSHDB?

Yes, for example by using a big enough bounding box, so that clipping doesn't affect the elements one is interested in. Or by using a custom TagInterpreter, or by using the low level data access of the OSHDB instead of the oshdb-api.

@tyrasd tyrasd added the priority:high Should be addressed as soon as possible (next release) label Mar 23, 2021
@FabiKo117
Copy link
Contributor

I see.. and yes of course we can only deal with what JTS can deal with in that matter. The workaround with just using a bigger bbox would already be sufficient I guess.

Do we potentially have to add another test or extend one of the existing ones to also cover this specific issue? Because you say that it was something that was working as intended in v0.5.

Copy link
Member

@joker234 joker234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me. Only the question by @FabiKo117 is still an open question I would be interested in:

Do we potentially have to add another test or extend one of the existing ones to also cover this specific issue? Because you say that it was something that was working as intended in v0.5.

@tyrasd
Copy link
Member Author

tyrasd commented Mar 23, 2021

Do we potentially have to add another test or extend one of the existing ones to also cover this specific issue?

Unfortunately, it's hard to find data which triggers this bug in our limited test data set. 😬

@tyrasd tyrasd merged commit 12b2039 into master Mar 23, 2021
@tyrasd tyrasd deleted the fix-faultIntolerantSplitting branch March 23, 2021 18:06
@tyrasd tyrasd removed the waiting for review This pull request needs a code review label Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as expected priority:high Should be addressed as soon as possible (next release)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants