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 Hausdorff Distance trait #1041

Merged
merged 7 commits into from
Jul 31, 2023
Merged

Add Hausdorff Distance trait #1041

merged 7 commits into from
Jul 31, 2023

Conversation

JosiahParry
Copy link
Contributor

@JosiahParry JosiahParry commented Jul 26, 2023

  • 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 a new trait HausdorffDistance which is implemented for all geometries.

This is my first Rust library PR so I'll need some guidance getting this through the finish line!

Outstanding questions for maintainers:

  • This PR is lacking tests. Should there be a test for each combo of geometry?
  • CHANGES.md does not have a development version listed, how should it be modified?
  • what amount of documentation is needed / suggested?

geo/src/algorithm/hausdorff_distance.rs Outdated Show resolved Hide resolved
geo/src/algorithm/hausdorff_distance.rs Outdated Show resolved Hide resolved
@JosiahParry
Copy link
Contributor Author

@frewsxcv thanks! I've added a note in the documentation that it uses euclidean distance. Is this explicit enough?

https://github.com/JosiahParry/geo/blob/1c44e3b5578b4e2af24bf3755827ca2b87c7e2eb/geo/src/algorithm/hausdorff_distance.rs#L9-L10

I've also added a handful of tests as well. Please let me know if this is sufficient or if there ought to be exhaustive tests (every combo of geometry types).

@JosiahParry
Copy link
Contributor Author

I've pushed a commit that addresses formatting using cargo fmt.

@JosiahParry JosiahParry requested a review from frewsxcv July 27, 2023 16:09
geo/src/algorithm/hausdorff_distance.rs Outdated Show resolved Hide resolved
geo/src/algorithm/mod.rs Show resolved Hide resolved
@JosiahParry
Copy link
Contributor Author

I've added a note to the changelog. Please let me know what else is needed to close this PR. Thanks! :)

@michaelkirk
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Jul 31, 2023
1041: Add Hausdorff Distance trait  r=michaelkirk a=JosiahParry

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/main/CODE_OF_CONDUCT.md).
- [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.

---

This PR adds a new trait `HausdorffDistance` which is implemented for all geometries. 

This is my first Rust library PR so I'll need some guidance getting this through the finish line! 

### Outstanding questions for maintainers: 

- This PR is lacking tests. Should there be a test for each combo of geometry?
- `CHANGES.md` does not have a development version listed, how should it be modified?
- what amount of documentation is needed / suggested? 

Co-authored-by: Josiah Parry <josiah.parry@gmail.com>
@bors
Copy link
Contributor

bors bot commented Jul 31, 2023

This PR was included in a batch that successfully built, but then failed to merge into main. It will not be retried.

Additional information:

Response status code: 422
{"message":"Changes must be made through the merge queue","documentation_url":"https://docs.github.com/articles/about-protected-branches"}

@michaelkirk
Copy link
Member

bors retry

Whoops. My bad I think. Let's try again now that I have (for real this time) disabled the merge queue.

@bors
Copy link
Contributor

bors bot commented Jul 31, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit eeb2b8e into georust:main Jul 31, 2023
@JosiahParry JosiahParry deleted the hausdorff branch July 31, 2023 18:37
@michaelkirk
Copy link
Member

Thanks!

@JosiahParry
Copy link
Contributor Author

Thank you!

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.

4 participants