Geometry dimensions via HasDimensions trait #524
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
michaelkirk
commented
Oct 20, 2020
| /// assert!(rect.get_dimensions() > line_string.get_dimensions()); | ||
| /// ``` | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd)] | ||
| pub enum Dimensions { |
There was a problem hiding this comment.
"Dimensionality" would probably be a more "correct" term for this enum, but I thought maybe it was too verbose.
I could be persuaded!
There was a problem hiding this comment.
It's our code, and we do what we feel like. (There's no ambiguity in "dimensions", so there's no problem imo)
michaelkirk
commented
Oct 20, 2020
| pub enum Dimensions { | ||
| /// Some geometries, like a `MultiPoint` or `GeometryColletion` may have no elements - thus no | ||
| /// dimensions. Note that this is distinct from being `ZeroDimensional`, like a `Point`. | ||
| Empty, |
There was a problem hiding this comment.
I considered ommitting this and instead representing Empty as a None Option<Dimensions>, but it seemed to work better with Empty as an explicit member of the enum.
Primarily because I want to use it in comparisons:
if a.get_dimensions() < Dimensions::OneDimensional {
do_something()
} else {
do_something_else()
}
is preferable to something like:
match a.get_dimensions() {
Some(dimension) => {
if dimensions < Dimensions::OneDimensional {
do_something()
} else {
do_something_else()
}
}
None => { do_something_else() }
}
frewsxcv
approved these changes
Oct 21, 2020
geo/src/algorithm/dimensions.rs
Outdated
| /// assert_eq!(Dimensions::Empty, empty_collection.get_dimensions()); | ||
| /// assert!(empty_collection.get_dimensions() < point.get_dimensions()); | ||
| /// ``` | ||
| fn get_dimensions(&self) -> Dimensions; |
There was a problem hiding this comment.
thoughts on removing the get_ prefix? getters in rust tend to not have 'get'
https://rust-lang.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter
There was a problem hiding this comment.
Love it! Honestly I assumed you couldn't have a method and property with the same name. 😅
michaelkirk
force-pushed
the
mkirk/has-dimensions
branch
3 times, most recently
from
c4f63d0 to
3ecda61
Compare
rmanoka
reviewed
Oct 21, 2020
geo/src/algorithm/dimensions.rs
Outdated
|
|
||
| impl<C: CoordinateType> HasDimensions for MultiLineString<C> { | ||
| fn is_empty(&self) -> bool { | ||
| if self.0.is_empty() { |
There was a problem hiding this comment.
Why is this needed? Iterator::all returns true on empty iterator (Iterator::any returns false).
There was a problem hiding this comment.
Good point - removed!
|
I think I've addressed all your feedback here as well @rmanoka. PTAL |
|
@rmanoka - I believe I've addressed your feedback. Unless I hear that you'd like more time to review, I'll merge in 24 hours. |
rmanoka
approved these changes
Oct 26, 2020
|
@michaelkirk lgtm, please go ahead. |
michaelkirk
force-pushed
the
mkirk/has-dimensions
branch
from
1ab39e5 to
d9c4b30
Compare
|
bors r=rmanoka |
|
Build succeeded: |
2 tasks
bors bot
added a commit
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>
bors bot
added a commit
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some efforts extracted from the geomgraph work that seemed generally useful, and thus worth separating.
(I'm also trying to do what I can to keep the upcoming geomgraph PR just a little bit smaller.)
Note this PR represents separate functionality from #523, but it relies on #523 for correct behavior, so I'm targeting #523 as the base branch. Before merging we should update the base to target master.