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

Integrate spade #1083

Merged
merged 19 commits into from
Nov 9, 2023
Merged

Integrate spade #1083

merged 19 commits into from
Nov 9, 2023

Conversation

RobWalt
Copy link
Contributor

@RobWalt RobWalt commented Oct 9, 2023

This PR aims at bringing spade triangulations to geo. More specifically I'm looking into implementing constrained delauny triangulations for geo since it seems handy in a lot of situations and opens up possibilities for new algorithms.

Todos

  • more tests?

Commit summary


chore: add spade as a dependency

although spade is a optional dependency, it looks like a good fit for the default features. Including it has the effect of bringing in 3 new dependencies:

  • robust 0.2 (we already use this on version 1.1)
  • optional 0.5
  • spade 2.2

I'll try to speak to the spade maintainers. Maybe we can bump the robust version in their crate to reduce dependencies further. Also it might make sense to ask them if the optional crate provides any real value.

Update: I created PRs for the changes

  • the robust version bump should make it since it comes with performance improvements
  • the optional removal might make it depending on the maintainers taste

Update: Both PRs got merged, so this change doesn't add any additional dependencies


feat: initial implementation of TriangulateSpade trait

includes all the code for the integration of spade into geo and also features some tests


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

@urschrei
Copy link
Member

urschrei commented Oct 9, 2023

This is great, @RobWalt, thank you! We'll wait to see how the proposed changes to spade go.

@RobWalt RobWalt mentioned this pull request Oct 11, 2023
6 tasks
@RobWalt RobWalt changed the title Integrate spade Draft: Integrate spade Oct 28, 2023
@RobWalt RobWalt marked this pull request as draft October 28, 2023 19:27
@RobWalt
Copy link
Contributor Author

RobWalt commented Oct 28, 2023

@urschrei Do you think it would be feasible to review & merge this as is and keep a tracking issue open for updating the dependencies which track the two spade PRs? I assume these two PRs are not going to be included anytime soon and this issue here is required to merge the spade boolops at some point

@urschrei
Copy link
Member

I'm in favour of merging this, both because it's a pre-requisite for the new boolean ops functionality, and because geo should support Delaunay triangulation as a feature more generally. For some reason it's not currently showing up as a top-level item under the algorithms module – again, you can check for yourself by running cargo doc --open.

Copy link
Member

@urschrei urschrei left a comment

Choose a reason for hiding this comment

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

Another thing that's needed here is a doctest example of how to use the algorithm to produce both constrained and unconstrained triangulations. This example should link to a good overview of what is meant by constrained / unconstrained, although your diagrams are also great.

@RobWalt RobWalt force-pushed the feat/integrate-spade branch from 9e95957 to 2049576 Compare October 31, 2023 19:15
@RobWalt
Copy link
Contributor Author

RobWalt commented Oct 31, 2023

  • applied the review comments suggestions
  • rebased on main
  • sealed the helper methods on a separate private trait since they probably shouldn't be public in the first place

RobWalt and others added 12 commits October 31, 2023 20:17
although spade is a optional dependency, it looks like a good fit for
the default features. Including it has the effect of bringing in 3 new
dependencies:

- robust 0.2 (we already use this)
- optional 0.5
- spade 2.2

I'll try to speak to the spade maintainers. Maybe we can bump the robust
version in their crate to reduce dependencies further. Also it might
make sense to ask them if the optional crate provides any real value.
@RobWalt RobWalt force-pushed the feat/integrate-spade branch from 2049576 to e29f3eb Compare October 31, 2023 19:17
@RobWalt RobWalt marked this pull request as ready for review October 31, 2023 19:17
@RobWalt RobWalt requested a review from urschrei October 31, 2023 19:18
@RobWalt RobWalt changed the title Draft: Integrate spade Integrate spade Oct 31, 2023
@urschrei urschrei requested a review from rmanoka November 1, 2023 15:02
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.

Nice work! None of my comments should be considered blocking.

@RobWalt
Copy link
Contributor Author

RobWalt commented Nov 2, 2023

Thanks for the thorough review @michaelkirk! 🏅 I like your taste 👍🏼

@RobWalt
Copy link
Contributor Author

RobWalt commented Nov 2, 2023

Ok, the proposed changes to spade were accepted and are merged as of now. There's nothing major in the way now. The only things left are:

  • explore review comment about epsilon
  • add more tests (?)

Copy link
Contributor

@rmanoka rmanoka left a comment

Choose a reason for hiding this comment

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

LGTM; thanks @RobWalt .

@urschrei urschrei added this pull request to the merge queue Nov 9, 2023
Merged via the queue into georust:main with commit ff1fd16 Nov 9, 2023
@RobWalt
Copy link
Contributor Author

RobWalt commented Nov 9, 2023

😎

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