Add GeodesicIntermediate algorithm#608
Conversation
michaelkirk
left a comment
michaelkirk
left a comment
There was a problem hiding this comment.
A couple minor cleanups requested, but the functionality looks great!
|
Thanks for the comments, just pushed some updates. |
michaelkirk
left a comment
michaelkirk
left a comment
There was a problem hiding this comment.
This looks good - thank you @profil!
I'm going to wait a couple days before merging in case anyone else wants to review.
In the meanwhile, could you push up a commit to advertise this new feature in geo/CHANGES.md?
|
Alright, cool! Thanks for quick feedback 😃 |
|
I have had a superficial look at this .. and it looks good. I am busy at the moment...I won't have time until the weekend to do a proper review. If it helps, I will sit down first thing sat morning (uk time [ utc + 0] ) . with a pot of coffee and do something better. |
|
sorry by 'do something better' I mean in terms of review... the code quality looks good. |
martinfrances107
left a comment
martinfrances107
left a comment
There was a problem hiding this comment.
I have looked over the algorithm carefully and could find only minor nits
Otherwise this looks really good.
| g.direct(self.lat(), self.lng(), azi1, total_distance * current_step); | ||
| let point = Point::new(lon2, lat2); | ||
| points.push(point); | ||
| current_step = current_step + interval; |
There was a problem hiding this comment.
We have recently removed all warnings generated by running 'cargo clippy' on the project.
Its a really minor point but this PR introduced a single clippy warning. The solution is the change this line : -
current_step = current_step + interval;
into
current_step+=interval
There was a problem hiding this comment.
Of course! TIL about clippy
| /// # Examples | ||
| /// | ||
| /// ``` | ||
| /// # #[macro_use] extern crate approx; |
There was a problem hiding this comment.
I could not justify these line
- /// # #[macro_use] extern crate approx;
- /// #
when I ran cargo doc and looked at the generated HTML
geo/target/doc/geo/algorithm/geodesic_intermediate/trait.GeodesicIntermediate.html
It did not appear in the output... perhaps we can just simplify by removing them.
There was a problem hiding this comment.
Within a doc comment code block, lines starting with a # denote that the line should not be output into the docs, but they are executed (and necessary) when running the doc-test.
It's commonly used to keep the rendered docs concise and focused on the usage, rather than implementation details like what testing library we use.
|
Thanks for looking @martinfrances107 - once the one nit is addressed, I'll merge posthaste. |
|
Just had my evening coffee and fixed the nit. Thanks for reviewing! |
|
bors r+ |
|
Build succeeded: |
4 participants
CHANGES.mdif knowledge of this change could be valuable to users.Hello!
I was in the need of generating accurate points on a line (just like https://pyproj4.github.io/pyproj/stable/api/geod.html#pyproj.Geod.npts), so I might as well submit a PR with my implementation.
Borrowing heavily from the haversine intermediate module.