-
Notifications
You must be signed in to change notification settings - Fork 199
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
Clarification Needed on Bearing Output Range #1210
Comments
Better documentation is always appreciated. |
To make this even more confusing, I noticed HaversineBearing and GeodesicBearing return -180..180, while Rhumb returns 0..360! I'm going to take a stab at unifying this as part of #1181 My understanding is the same as yours - that it is more common for GIS libraries to represent west as 270, rather than -90, so the new traits I'm introducing as part of #1181 will switch to that convention. Since it's introducing a new set of traits/methods, it seems like a good time for this break. And I'll document it. =) |
I recently discovered another unexpected (for me) behaviour that isn't explicitly documented in either |
Interesting. I'm not really sure what to expect in that situation. Just thinking about it conceptually, it seems like bearing is undefined in the case of no movement. But if there is a coherent argument for any particular value, I'd consider adopting it. |
Yes, the bearing for a stationary point is technically undefined. Using Other alternatives, such as using |
All of these algorithm implementations already existed, but I'm proposing a new way to organize them which I hope will be more consistent and discoverable. Note: The new Haversine::bearing and Geodesic::bearing return 0..360, the legacy traits HaversineBearing and GeodesicBearing returned -180..180 Additional changes: Deleted the deprecated `Bearing` trait which was previously superceeded by the unambiguous `HaversineBearing` trait, but now is re-defined as `Haversine::bearing` = Future Work = In an effort to minimize this PR, while keeping the change reasonably coherent, I've left some things out == Methods on Euclidean == -[ ] bearing (doesn't currently exist) -[ ] destination (doesn't currently exist) -[ ] intermediate (exists, but InterpolatePoint trait also needs intermediate_fill) -[ ] intermediate_fill (doesn't currently exist) == Deprecate Legacy Traits == -[ ] Deprecate Legacy impls -[ ] Switcheroo the actual implementation: move the actual implementation to the new traits, and have the legacy traits delegate to the new traits. -[ ] Move over any tests from the legacy implementation to the new home == Methods on Geoms (Future PR) == -[ ] Length -[ ] Haversine -[ ] Rhumb -[ ] Geodesic -[ ] Euclidean -[ ] Densify -[ ] Haversine -[ ] Rhumb -[ ] Geodesic -[ ] Euclidean FIXES #1210 FIXES #1181
All of these algorithm implementations already existed, but I'm proposing a new way to organize them which I hope will be more consistent and discoverable. Note: The new Haversine::bearing and Geodesic::bearing return 0..360, the legacy traits HaversineBearing and GeodesicBearing returned -180..180 Additional changes: Deleted the deprecated `Bearing` trait which was previously superceeded by the unambiguous `HaversineBearing` trait, but now is re-defined as `Haversine::bearing` = Future Work = In an effort to minimize this PR, while keeping the change reasonably coherent, I've left some things out == Methods on Euclidean == -[ ] bearing (doesn't currently exist) -[ ] destination (doesn't currently exist) -[ ] intermediate (exists, but InterpolatePoint trait also needs intermediate_fill) -[ ] intermediate_fill (doesn't currently exist) == Deprecate Legacy Traits == -[ ] Deprecate Legacy impls -[ ] Switcheroo the actual implementation: move the actual implementation to the new traits, and have the legacy traits delegate to the new traits. -[ ] Move over any tests from the legacy implementation to the new home == Methods on Geoms (Future PR) == -[ ] Length -[ ] Haversine -[ ] Rhumb -[ ] Geodesic -[ ] Euclidean -[ ] Densify -[ ] Haversine -[ ] Rhumb -[ ] Geodesic -[ ] Euclidean FIXES #1210 FIXES #1181
All of these algorithm implementations already existed, but I'm proposing a new way to organize them which I hope will be more consistent and discoverable. Note: The new Haversine::bearing and Geodesic::bearing return 0..360, the legacy traits HaversineBearing and GeodesicBearing returned -180..180 Additional changes: Deleted the deprecated `Bearing` trait which was previously superceeded by the unambiguous `HaversineBearing` trait, but now is re-defined as `Haversine::bearing` = Future Work = In an effort to minimize this PR, while keeping the change reasonably coherent, I've left some things out == Methods on Euclidean == -[ ] bearing (doesn't currently exist) -[ ] destination (doesn't currently exist) -[ ] intermediate (exists, but InterpolatePoint trait also needs intermediate_fill) -[ ] intermediate_fill (doesn't currently exist) == Deprecate Legacy Traits == -[ ] Deprecate Legacy impls -[ ] Switcheroo the actual implementation: move the actual implementation to the new traits, and have the legacy traits delegate to the new traits. -[ ] Move over any tests from the legacy implementation to the new home == Methods on Geoms (Future PR) == -[ ] Length -[ ] Haversine -[ ] Rhumb -[ ] Geodesic -[ ] Euclidean -[ ] Densify -[ ] Haversine -[ ] Rhumb -[ ] Geodesic -[ ] Euclidean FIXES #1210 FIXES #1181
Closing the loop on this - the new line measure traits in #1216 uniformly output a bearing in 0...360. I decided to leave the legacy traits as they are (with haversine and geodesic outputting -180...180), in hopes of surprising fewer people. #1222 deprecates the legacy traits, so they'd go away at some point. |
A lot of the line measure traits were re-worked. Mostly this is just moving code around in hopes of similar methods across metric spaces being easier to find now that they are uniform. It also enabled replacing some mostly copy/pasted implementations in geo with generic implementations, which enabled some new functionality - e.g. we can now `.densify::<Geodesic>` One notable behavioral change: the output of the new `Bearing` trait is now uniformly 0...360. Previously GeodesicBearing and HaversineBearing returned -180..180 while RhumbBearing returned 0...360. georust/geo#1210 This actually uncovered a bug in ferrostar, reflected in the new output of ferrostar/src/snapshots/ferrostar__simulation__tests__state_from_polyline.snap Since previously we were casting a potentially negative number to u16 — now it's always positive (0...360).
* Update to geo v0.29.0 A lot of the line measure traits were re-worked. Mostly this is just moving code around in hopes of similar methods across metric spaces being easier to find now that they are uniform. It also enabled replacing some mostly copy/pasted implementations in geo with generic implementations, which enabled some new functionality - e.g. we can now `.densify::<Geodesic>` One notable behavioral change: the output of the new `Bearing` trait is now uniformly 0...360. Previously GeodesicBearing and HaversineBearing returned -180..180 while RhumbBearing returned 0...360. georust/geo#1210 This actually uncovered a bug in ferrostar, reflected in the new output of ferrostar/src/snapshots/ferrostar__simulation__tests__state_from_polyline.snap Since previously we were casting a potentially negative number to u16 — now it's always positive (0...360). * adapt to new geo::Bearing output * Clarifying comments for the index_of_closest_segment_origin function * Fix debug assert range * Use haversine_closest_ponit --------- Co-authored-by: Ian Wagner <ian.wagner@stadiamaps.com>
Bearing (azimuth) is typically measured either in the range of 0° to 360° or in the signed range of -180° to +180°. After exploring the relevant methods in the crate, I discovered that they produce results in the signed range of -180° to +180°. However, this is not clearly stated in the current documentation.
To prevent any confusion or incorrect assumptions by users, I suggest updating the documentation to explicitly mention that the output of these methods lies in the signed range.
If this concern is valid, I am willing to create a pull request to address this documentation update.
The text was updated successfully, but these errors were encountered: