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.

Sign up for GitHub

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

Jump to bottom

Clarification Needed on Bearing Output Range #1210

Closed
grevgeny opened this issue Aug 19, 2024 · 6 comments · Fixed by #1216
Closed

Clarification Needed on Bearing Output Range #1210

grevgeny opened this issue Aug 19, 2024 · 6 comments · Fixed by #1216

Comments

@grevgeny
Copy link
Contributor

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.
@michaelkirk
Copy link
Member

Better documentation is always appreciated.

@michaelkirk
Copy link
Member

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. =)

@grevgeny
Copy link
Contributor Author

I recently discovered another unexpected (for me) behaviour that isn't explicitly documented in either geographic-lib or geo. While other bearing calculations return 0 for two identical points, GeodesicBearing returns +180 in such cases.

@michaelkirk
Copy link
Member

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.

@grevgeny
Copy link
Contributor Author

Yes, the bearing for a stationary point is technically undefined. Using 0.0 is not ideal as well since it can be a valid bearing for non-equivalent points, which could cause confusion. However, it might still be better to consistently fallback to a specific value (such as 0.0), provided that it's clearly documented.

Other alternatives, such as using Option, -1.0, or NaN, would provide more precise semantics but might be considered more disruptive in terms of breaking changes. And the current default value of 180.0 for geodesic bearing seems to be the least intuitive in this context.

michaelkirk added a commit that referenced this issue Sep 25, 2024
@michaelkirk
Unify line measures
a635f84 
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
michaelkirk added a commit that referenced this issue Sep 25, 2024
@michaelkirk
Unify line measures
a34590a 
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
michaelkirk added a commit that referenced this issue Sep 25, 2024
@michaelkirk
Unify line measures
c54af80 
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
@michaelkirk michaelkirk mentioned this issue Sep 25, 2024
Merged
2 tasks
@michaelkirk michaelkirk mentioned this issue Oct 8, 2024
Merged
2 tasks
@michaelkirk
Copy link
Member

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.

michaelkirk added a commit to michaelkirk/ferrostar that referenced this issue Oct 30, 2024
@michaelkirk
Update to geo v0.29.0
07f80c0 
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).
@michaelkirk michaelkirk mentioned this issue Oct 30, 2024
Merged
ianthetechie added a commit to stadiamaps/ferrostar that referenced this issue Oct 31, 2024
@michaelkirk @ianthetechie
[common] Update to georust/geo v0.29.0 (#328)
4b883ae 
* 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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Unify line measures georust/geo
2 participants
@michaelkirk @grevgeny