Skip to content
This repository has been archived by the owner on Apr 2, 2022. It is now read-only.

Add relate tests #6

Merged
merged 6 commits into from
Apr 13, 2021
Merged

Add relate tests #6

merged 6 commits into from
Apr 13, 2021

Conversation

michaelkirk
Copy link
Member

This PR adds support for the relate tests.

The sister PR which implements this functionality in geo is at georust/geo#639

Copy link
Contributor

@rmanoka rmanoka left a comment

Choose a reason for hiding this comment

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

Looks great! I didn't get to running the tests, but I assume all the tests are now passing. Nice work @michaelkirk !

@michaelkirk
Copy link
Member Author

I didn't get to running the tests, but I assume all the tests are now passing.

Yep, all the tests pass, though be aware of one peculiarity -

cd jts-test-runner; cargo test runs more JTS test cases than cd geo; cargo test

This is because the jts-test-runner/Cargo.toml is patched to point to your forked WKT which can parse more tests.

All the tests pass in either case, but something to be aware of.

@rmanoka
Copy link
Contributor

rmanoka commented Mar 31, 2021

I realize we don't yet test the Contains trait directly here (as it doesn't have all combinations implemented yet). For now, I tried to manually match the subject and target in Operation::Contains like so:

let verify_contains_trait = match (subject, target) {
    (Geometry::Point(subject), Geometry::Point(target)) => { subject.contains(target) == relate_actual }
    // ... all combinations that have contains implementation ...
    _ => { true }
};

Fortunately, it didn't trip any of the tests, so that's good.

Add checks to verify Contains and Relate are the same
wherever possible.
@rmanoka
Copy link
Contributor

rmanoka commented Mar 31, 2021

@michaelkirk If it makes sense, you could merge this PR into this branch before merging to master. It just manually checks the Contains implementation for the combinations where we have the trait implementation. This is useful as we don't always use Relate in impl Contains. It might also come in handy to check which combinations are yet to be implemented.

@michaelkirk
Copy link
Member Author

This is useful as we don't always use Relate in impl Contains. It might also come in handy to check which combinations are yet to be implemented.

Good call. Done and thank you!

bors bot added a commit to georust/geo that referenced this pull request Apr 13, 2021
639: Introduce the geomgraph module for DE-9IM Relate trait r=michaelkirk a=michaelkirk

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md).
- [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---

Fixes #513, #515

(I'm sorry it's so large)

~~I'm going to leave it as a draft (edit: 🤦 I failed to actually open the PR as a draft) while I wait to merge #636 and #638 and then do some rebasing, but I don't anticipate doing other large changes before review.~~ *update: ready for review!*

Here's some of the earlier work in pursuit of this:

#514
#516
#523
#524 
#538
#552
#561
#611
#628
#629
#636 

Primarily, this introduces the geomgraph module for a DE-9IM `Relate` trait.

geomgraph implements a topology graph largely inspired by JTS's module of the same name:
https://github.com/locationtech/jts/tree/jts-1.18.1/modules/core/src/main/java/org/locationtech/jts/geomgraph

You can see some of the reference code if you omit the "REMOVE JTS COMMENTS" commit. In some places the implementation is quite close to the JTS source. 

The overall "flow" is pretty similar to that of JTS, but in the small, there were some divergences. It's not easy (or desirable) to literally translate a Java codebase making heavy use of inheritance and pointers to rust. Additionally, I chose to take advantage of `Option` and rust's enums with associated data to make some case analysis more explicit.

There is a corresponding PR in our [jts-test-runner](georust/jts-test-runner#6) crate which includes the bulk of the tests for the new Relate trait.

## Algorithm Overview 

This functionality is accessed on geometries, via the `Relate` trait, e.g. `line.relate(point)` which returns a DE-9IM [`IntersectionMatrix`](https://en.wikipedia.org/wiki/DE-9IM#Matrix_model).

The `Relate` trait is driven by the `RelateOperation`. The `RelateOperation` builds a `GeometryGraph` for each of the two geometries being related. 

A `GeometryGraph` is a systematic way to organize the "interesting" parts of a geometry's structure - e.g. where its vertices, lines, and areas lie relative to one another.

Once the `RelateOperation` has built the two `GeometryGraph`s, it uses them to efficiently compare the two Geometries's structures, outputting the `IntesectionMatrix`.





Co-authored-by: Michael Kirk <[email protected]>
Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com>
@michaelkirk michaelkirk merged commit 4a69a55 into master Apr 13, 2021
bors bot added a commit to georust/geo that referenced this pull request Apr 13, 2021
639: Introduce the geomgraph module for DE-9IM Relate trait r=frewsxcv,rmanoka a=michaelkirk

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md).
- [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---

Fixes #513, #515

(I'm sorry it's so large)

~~I'm going to leave it as a draft (edit: 🤦 I failed to actually open the PR as a draft) while I wait to merge #636 and #638 and then do some rebasing, but I don't anticipate doing other large changes before review.~~ *update: ready for review!*

Here's some of the earlier work in pursuit of this:

#514
#516
#523
#524 
#538
#552
#561
#611
#628
#629
#636 

Primarily, this introduces the geomgraph module for a DE-9IM `Relate` trait.

geomgraph implements a topology graph largely inspired by JTS's module of the same name:
https://github.com/locationtech/jts/tree/jts-1.18.1/modules/core/src/main/java/org/locationtech/jts/geomgraph

You can see some of the reference code if you omit the "REMOVE JTS COMMENTS" commit. In some places the implementation is quite close to the JTS source. 

The overall "flow" is pretty similar to that of JTS, but in the small, there were some divergences. It's not easy (or desirable) to literally translate a Java codebase making heavy use of inheritance and pointers to rust. Additionally, I chose to take advantage of `Option` and rust's enums with associated data to make some case analysis more explicit.

There is a corresponding PR in our [jts-test-runner](georust/jts-test-runner#6) crate which includes the bulk of the tests for the new Relate trait.

## Algorithm Overview 

This functionality is accessed on geometries, via the `Relate` trait, e.g. `line.relate(point)` which returns a DE-9IM [`IntersectionMatrix`](https://en.wikipedia.org/wiki/DE-9IM#Matrix_model).

The `Relate` trait is driven by the `RelateOperation`. The `RelateOperation` builds a `GeometryGraph` for each of the two geometries being related. 

A `GeometryGraph` is a systematic way to organize the "interesting" parts of a geometry's structure - e.g. where its vertices, lines, and areas lie relative to one another.

Once the `RelateOperation` has built the two `GeometryGraph`s, it uses them to efficiently compare the two Geometries's structures, outputting the `IntesectionMatrix`.





Co-authored-by: Michael Kirk <[email protected]>
Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants