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

Add LineStringSegmentizeHaversine #1107

Merged
merged 8 commits into from
Dec 5, 2023
Merged

Add LineStringSegmentizeHaversine #1107

merged 8 commits into from
Dec 5, 2023

Conversation

JosiahParry
Copy link
Contributor

@JosiahParry JosiahParry commented Nov 5, 2023 •
edited
Loading

  • 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.

Supercedes #1106.

This PR:

  • adds a new trait LineStringSegmentizeHaversine to segment a linestring using haversine distance instead of euclidean distance
    • this gives downstream users an option to segment linestrings on geographic as well as projected coordinates
  • fixes a bug in LineStringSegmentize where n - 1 linestrings were created for very small distances
    • this was caused by precision in the 6-ish decimal place. It was fixed by subtracting f64::EPSILON from the calculated segment length that is passed to densify()
  • turns the segmentize implementation into a macro.
    • the benefit of this is that segmentize for haversine does not need to duplicate code and other future implementations (hopefully geodesic) will not need more than a Densify and Length trait of the same type.

@JosiahParry
fix: n - 1 linestrings at small distances
8514209
@JosiahParry
format test
ff68f7e
@JosiahParry
rewrite doc test to not hard code coords
0520e94
@JosiahParry
fix: Bug where n - 1 line segments were being created at very small
eb82e2f 
     distances. Caused by rounding error in division. Fixed by
     subtracting f64::EPSILON from the calculated segment length
     before passing into densify.

feature: adds `LineStringSegmentizeHaversine` to provide geographic
     support for segmentizing linestrings.

misc: segmentize traits are now implemented via a macro. This will
     make it easier to add additional segmentize traits in the future.
     A geodesic one would be preferable but would require a new
     densify trait.
@JosiahParry
update changelog
73e5435
@JosiahParry
update doc
70fac48
This was referenced Nov 5, 2023
Closed
Closed
@JosiahParry
Copy link
Contributor Author

Please let me know if this needs any additional work. :)

frewsxcv
frewsxcv approved these changes Dec 4, 2023
View reviewed changes
Copy link
Member

@frewsxcv frewsxcv left a comment

Choose a reason for hiding this comment

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

lgtm! will see if others have thoughts before merging

@frewsxcv frewsxcv added this pull request to the merge queue Dec 5, 2023
Merged via the queue into georust:main with commit c988af4 Dec 5, 2023
15 checks passed
@JosiahParry
Copy link
Contributor Author

thanks for your help getting this across the line! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frewsxcv frewsxcv frewsxcv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JosiahParry @frewsxcv