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

Implement topological equality check for IntersectionMatrix #1133

Merged
merged 7 commits into from
Jan 10, 2024

Conversation

urschrei
Copy link
Member

@urschrei urschrei commented Jan 1, 2024

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

Fixes #1132

@urschrei
Copy link
Member Author

urschrei commented Jan 1, 2024

@michaelkirk There are some cases that will be useful for this in the jts-test-runner Relate suite (TestRelateLC and TestRelateLL), but even after looking at the README I can't figure out how to activate / write them.

@michaelkirk
Copy link
Member

All the files in textxml/general are parsed and processed. We only skip the operations we don't know how to support which may live across files or be a subset of a particular file. This seemed like the best way to get as much test coverage as possible.

I hope you don't mind, but I pushed a commit to your branch that prints a little bit more about the stats of how many test cases are being skipped and why in jts_test_runner. You can see a lot more detail (too much detail probably) if you run with RUST_LOG=jts_test_runner=debug cargo test -p jts-test-runner

Feel free to remove it if you don't want to include it in your branch.

@michaelkirk
Copy link
Member

Just to be clear - the tests in TestRelateLC and TestRelateLL are already being run. You can convince yourself by watching the tests fail after you tweak the expected output.

@urschrei
Copy link
Member Author

urschrei commented Jan 2, 2024

Ahhh OK. Thank you! I'll have a further poke around.

@michaelkirk
Copy link
Member

re: the CI failure, I'll open a PR soon that bumps our MSRV to 1.70 (>6 months old) and updates the "latest" containers.

self.0[CoordPos::Inside][CoordPos::Inside] != Dimensions::Empty
&& self.0[CoordPos::Inside][CoordPos::Outside] == Dimensions::Empty
&& self.0[CoordPos::Outside][CoordPos::Inside] == Dimensions::Empty
&& self.0[CoordPos::Outside][CoordPos::OnBoundary] == Dimensions::Empty
Copy link
Member

Choose a reason for hiding this comment

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

I think we need one more condition:

&& self.0[CoordPos::OnBoundary][CoordPos::Outside] == Dimensions::Empty

Copy link
Member Author

Choose a reason for hiding this comment

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

Damn, thought I had them all. I looked at the other is_ methods for inspiration, but evidently got a bit confused. Which matrix entry does it correspond to?

Copy link
Member

@michaelkirk michaelkirk Jan 3, 2024

Choose a reason for hiding this comment

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

TBH, I firstly just intuited that any checks for an "is equal" relationship should be symmetrical - and noticed they weren't.

Then I noticed there are 5 constraints in the de-9im spec string, but you only had 4 comparisons and deduced which non-symmetrical check was missing.

But to try to answer your question directly, I think it's like this:

Spec for "is topo equal": [T*F**FFF*]

Inside_a OnBoundary_a Outside_a
Inside_b T * F
OnBounary_b * * F (missing)
Outside_b F F *

Corresponding Indexes into de-9im string

Inside_a OnBoundary_a Outside_a
Inside_b 0 1 2
OnBounary_b 3 4 5 (missing)
Outside_b 6 7 8

So we were missing a check for the 6th element (which has an index of 5).

[T*F**FFF*]
[     ^   ]

Copy link
Member Author

Choose a reason for hiding this comment

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

Amazing, thank you!

@urschrei urschrei marked this pull request as ready for review January 10, 2024 08:21
@urschrei urschrei requested a review from michaelkirk January 10, 2024 08:46
@michaelkirk michaelkirk added this pull request to the merge queue Jan 10, 2024
Merged via the queue into georust:main with commit 8dc28a3 Jan 10, 2024
15 checks passed
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.

Add is_equal_topo method to IntersectionMatrix to determine topological equality
2 participants