Skip to content

Conversation

@3Davydov
Copy link
Contributor

@3Davydov 3Davydov commented Aug 4, 2023

No description provided.

@esabol
Copy link
Contributor

esabol commented Aug 4, 2023

Thank you for this, @3Davydov ! I love more tests.

There's already a "contains_ops" and a "contains_ops_compat" files for testing. I wonder if it would be appropriate to put your "contains" tests in one of those files?

@3Davydov
Copy link
Contributor Author

3Davydov commented Aug 4, 2023

Hello, @esabol !
I think it's still worth making a separate file for this. The tests in the "contains_ops" and "contains_ops_compat" files are relevant only for the current logic.
If it is decided to switch to the DE-9IM model, then it will be necessary first of all to focus on the tests from "contains", and it may be more convenient to work with a separate file.

Copy link
Contributor

@vitcpp vitcpp left a comment

Choose a reason for hiding this comment

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

@esabol I propose to accept this PR with some trivial changes. Tests do not affect functionality but can be used to learn semantics of operations. I have some ideas how to improve and reorganise it. @3Davydov did a nice analysis of existing algorithms and found that they are not fully compliant with the semantics of operations on figures as described in the DE-9IM model (and OSG Simple Feature Specification). I understand that we can not just change the behaviour of operations to make it compliant but we can update the docs with some notes about differences with reference models.

@3Davydov 3Davydov force-pushed the contains_and_overlaps_test branch from a8202a5 to 23ff77e Compare August 8, 2023 00:17
@vitcpp vitcpp merged commit 8a75bc2 into postgrespro:master Aug 8, 2023
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.

3 participants