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 test for GH issue 1064, for main branch #1093

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

strk
Copy link
Member

@strk strk commented May 7, 2024

See GH-1064

And see GH-1092 for the 3.8 runs

@dr-jts
Copy link
Contributor

dr-jts commented May 7, 2024

AFAIK there is no known fix for this. It's caused by rounding in the calculation of the intersection point (note, not the intersects test, which is accurate). So this test can't be added to CI at this time, since it will just keep failing.

Note that 3.8 is reporting that there is no intersection between the lines at all. The Orientation predicate however is reporting that there is a proper intersection between them (see image below). This uses the best known algorithm we have, so the only thing we can do is to accept it as the correct analysis of the situation. So in this case 3.8 is clearly wrong. The result of this test should be changed to be the result produced in later versions of GEOS (F01FF0102).

image

@strk strk marked this pull request as draft May 8, 2024 13:09
@strk
Copy link
Member Author

strk commented May 8, 2024

Rather than adding a broken expectance I'd expect the right answer, and I'd expect the test to fail.

If the picture you attached is correct the right answer should be: 0F1FF01F1, right ?

Maybe we can implement an "expected failure" that turns red in case the answer gets correct

@dr-jts
Copy link
Contributor

dr-jts commented May 8, 2024

Rather than adding a broken expectance I'd expect the right answer, and I'd expect the test to fail.
If the picture you attached is correct the right answer should be: 0F1FF01F1, right ?

What is the "right" answer is very subtle. The best we can say is based on whatever numerical method is used to evaluate the Orientation predicate. (It can also depend on the fundamental imprecise representation of decimal numbers - e.g. 0.9, as in #968.).

For current versions of GEOS the answer is as given above. Older versions use different arithmetic and get a different answer. I agree there should be a "correct" answer - but I don't know how to calculate it (do you?). And in any case, even if the computed answer is wrong, it doesn't help us to have CI failing all the time.

Maybe we can implement an "expected failure" that turns red in case the answer gets correct

Yes, I've done this in JTS in a few places, mostly in order to capture cases which are known failures. But someone needs to figure out what the correct answer is in this case. At the moment all we have are two different answers, with no way to choose between them.

@dr-jts
Copy link
Contributor

dr-jts commented May 8, 2024

I'm not sure there's much point in adding tests like this, where there is not a "known good" answer. If we change (or improve) the orientation predicate algorithm, that may cause this test to start failing, but it won't provide a solid indication that something is wrong. So we'd end up just changing or deleting it.

I'm actually dealing with exactly this situation, with a change I'm considering for the orientation predicate. A bunch of tests fail with the new code, but they don't indicate it's wrong, just has different results. It's to handle #968 - and it does make that case work.

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