Skip to content

Fix or suppress validity test failures in set operations #550

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

Merged
merged 2 commits into from
Jan 30, 2019

Conversation

awulkiew
Copy link
Member

With this PR I intend to fix or suppress test failures in set operations.

There are several cases failing right now. Most of them are failing correctly due to a bug in sym_difference(): #515 and became visible because of this change: #533.

There is a test case which is different, i.e. difference() case mysql_23023665_6:

image
red: POLYGON((6 7,18 14, -8 1, 0 0, 18 -8, 6 7), (6 0, -4 3, 5 3, 6 0))
green: POLYGON((2 3,-3 5,-10 -1,2 3))
blue: difference(green, red);

This case is fixed by the first commit in this PR which changes the relate() algorithm to use rescaling just like set operations. In other words to use it for areal geometries. This make relate() just slightly more consistent with set operations. It's because in set operations rescaling done WRT bounding box of both geometries, e.g. 2 MultiPolygons but in relate() called in is_valid() rescaling is done WRT pairs of polygons tested for intersecting interiors.

Because of the above after the change in relate() other case starts to fail vailidity test, i.e.: sym_difference() and union() for MultiPolygons ticket_9081:

image

The problematic fragment is here:

image

The result of set operation depends on the size of a box used for rescaling. E.g. if union_() of only these 2 polygons is calculated the result is one polygon instead of two:

image

So I could pass rescaling policy generated for the whole MultiPolygon instead of a pair of Polygons in is_valid(), this might "fix" this issue or not. But even assuming it would fix this particular issue I could easily prepare a different test case which would fail due to this difference in rescaling. So I think for now I'll conditionally disable validity testing for this test case if rescaling is enabled. Ok?

@awulkiew
Copy link
Member Author

awulkiew commented Jan 24, 2019

Side note. I tried to enable rescaling for all geometries in relate(), also for linestrings and because of this found out that get_rescale_policy() may throw overflow numeric conversion exception here: https://github.com/boostorg/geometry/blob/develop/include/boost/geometry/policies/robustness/get_rescale_policy.hpp#L81. Because diff is very small but greater than machine epsilon and there is no upper limit to rescaled coordinates. This is one of the test cases in question: https://github.com/boostorg/geometry/blob/develop/test/algorithms/equals/equals.cpp#L93
For convenience:

equals(LINESTRING(-3.2333333333333333925452279800083 5.5999999999999978683717927196994,-3.2333333333333333925452279800083 5.5999999999999996447286321199499),
       LINESTRING(-3.2333333333333325043668082798831 5.5999999999999996447286321199499,-3.2333333333333333925452279800083 5.5999999999999996447286321199499))

EDIT: This fails for Linestrings and for Linestrings rescaling is disabled but I bet I could easily create very small Polygon which would cause the same issue.

@awulkiew
Copy link
Member Author

awulkiew commented Jan 25, 2019

Regarding other difference() cases that are failing, they all fail due to #515 (they all have the same problematic fragment). So I'm going to disable validity test for them as well as @barendgehrels suggested here: 247f657. Or do you have some other suggestion?

@awulkiew awulkiew added the bug label Jan 25, 2019
@awulkiew awulkiew added this to the 1.70 milestone Jan 25, 2019
@barendgehrels
Copy link
Collaborator

I'm OK with this change. Thank you.

@awulkiew awulkiew merged commit b4256a5 into boostorg:develop Jan 30, 2019
@awulkiew
Copy link
Member Author

Thanks. Now lets see if the tests are green again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants