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

Generic implementation of geodesic_destination for Point #1202

Closed
wants to merge 1 commit into from

Conversation

arpadav
Copy link

@arpadav arpadav commented Jul 26, 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.

Previously was only implemented for f64

Question: Not too happy about the unwrap's, any way to avoid? Or will the trait bounds of CoordFloat prevent to_f64 and from from ever being None?

@michaelkirk
Copy link
Member

There isn't anything meaningfully generic happening here. I can see how automatically converting to and from the underlying data type where all the calculations happen is a convenient API, but it could be misleading, and I don't like the additional unwraps.

Or will the trait bounds of CoordFloat prevent to_f64 and from from ever being None?

I'm not sure about that. Care to investigate?

A different approach would be to convert your geometries in your application code to f64 when you want to use these f64 methods. The MapCoords trait might be helpful there.

@arpadav
Copy link
Author

arpadav commented Jul 26, 2024

Ultimately agree with your sentiment. The last thing I want is to be misleading for the sake of convenience, and MapCoords looks promising despite the overhead

Can close, but can always revisit if geographiclib updates the WGS84 impl to use generics rather than f64. Or if someone re-writes the calculation without said dependency

@michaelkirk
Copy link
Member

Closing this based on previous discussion.

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.

2 participants