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

Add DensifyGeodesic trait #1208

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Add DensifyGeodesic trait #1208

wants to merge 12 commits into from

Conversation

grevgeny
Copy link

@grevgeny grevgeny commented Aug 10, 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.

This PR adds DensifyGeodesic trait that follows logic similar to DensifyHaversine. However, the trait is only implemented for T = f64 as underlying Geodesic traits are implemented only for this type.

Copy link
Member

@michaelkirk michaelkirk left a comment

Choose a reason for hiding this comment

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

I have some minor nits, but overall it looks good. Thank you!

geo/src/algorithm/densify_geodesic.rs Outdated Show resolved Hide resolved
geo/src/algorithm/densify_geodesic.rs Show resolved Hide resolved
geo/src/algorithm/mod.rs Outdated Show resolved Hide resolved
@grevgeny
Copy link
Author

@michaelkirk thanks for your review! I have added proposed changes

@michaelkirk
Copy link
Member

I think you'll need to run cargo fmt, otherwise LGTM

I'll leave it open for a couple days before merging in case anyone else wants to weigh in.

@grevgeny
Copy link
Author

@michaelkirk

I've just realised that there is already the following method: geodesic_intermediate_fill. It expresses pretty much the same logic.

Do we still need this another implementation?

@michaelkirk
Copy link
Member

Darn it. I'm sorry for not realizing this earlier. This seems related to #1181

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