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

Sg/in on updates #37

Closed
wants to merge 39 commits into from
Closed

Conversation

skygering
Copy link
Collaborator

I implemented a generalizable solution for most of the DE-9IM functions (https://en.wikipedia.org/wiki/DE-9IM).

The only one not there is equals because I already have a better (I think?) way of solving it.

I am having issues most with overlaps and crosses. Right now 3 overlaps tests are failing with regards to LibGEOS comparisons (line-line and line-ring). However, I am not sure I agree with the LibGEOS answer. In the example cases that are failing, the two curves DO overlap, but they also cross on different segments. In the DE-9IM functions guide, it says that for line-line crosses, interactions must be of dimension 1, so I thought that the cross, which creates a point, would exclude this example from passing.

Then for crosses there is a multi-geometry test failing. LibGEOS says a geometry collection crosses a multi-line string, but none of the individual elements cross so I don't know how that is possible.

I am open to debugging these, but I would like to merge this sooner rather than later because I have code that depends on some of this code. I probably should have done this in two PRs or something because it is a lot. Would it make sense maybe to move overlaps and crosses into a new PR?

@skygering skygering requested a review from rafaqz January 2, 2024 09:08

test_pairs = [
# Points and geometries
(pt1, pt1, "pt1", "pt1"), # Same point
Copy link
Member

Choose a reason for hiding this comment

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

Can we put these descriptions like "Same point" in thd tuple and print it with the other info?

@rafaqz
Copy link
Member

rafaqz commented Jan 2, 2024

Amazing I also wanted to do the same reorganisation.

Its not a huge issue but usually you want to keep file moves and code changes in separate PRs so the comparisons are side by side here. Its a bit hard to review this. You also want to use git mv rather than deleting files and adding new ones.

And did you mean line-line overlaps should be 1d? Probably LibGEOS is right somehow 😅

@skygering
Copy link
Collaborator Author

Whoops! Would you prefer that I split it into multiple PRs? That might help with me splitting out the overlap and crosses issues.

Here is the the case where overlaps is failing. It does overlap, but it also crosses. And from my understanding of the DE-9IM standards, overlaps of dimension 1 objects should be of dimension 1.
crosses_fail

Screenshot 2024-01-02 at 9 19 29 AM

Is something dimension 1 if it has any component that is dimension 1?

@skygering
Copy link
Collaborator Author

Okay, I am going to split this into 3 different PRs. I need some of the functionality and I think that is the easier part to merge. It'll also make it easier to review like you said above.

@skygering skygering closed this Jan 2, 2024
@rafaqz
Copy link
Member

rafaqz commented Jan 2, 2024

I think anything of dimension 1 is enough to be overlapping?

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.

2 participants