-
Notifications
You must be signed in to change notification settings - Fork 78
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
Return alpha option & use pygeos for alphashaping if available #278
Conversation
Codecov Report
@@ Coverage Diff @@
## master #278 +/- ##
==========================================
+ Coverage 80.99% 81.06% +0.07%
==========================================
Files 115 115
Lines 11580 11671 +91
==========================================
+ Hits 9379 9461 +82
- Misses 2201 2210 +9
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very promising!
This looks good to me too. My only question would be: have you done any check on whether performance when |
Sure, I can run timing experiments now. But, for what it's worth I think it's fine for two reasons.
There isn't really another way to implement this from an algorithmic perspective. Solely for the API/UX, we could only support building the circles in a second call using the constructor I outlined in the first post, |
whoops did not mean to close! |
For comparison, the timing table with pygeos:
the new lprun suggests that w/ pygeos, the breakdown is:
circle construction is still <.1% of total computation time. |
This is amazing!!! Super good report too! I think since it does not seem to affect much, and it's a nice feature, we can pull it in. The Very minor thing for consideration: maybe better to add a gpkg file for the tests, instead of a shapefile? |
I've migrated the data in the alpha shape tests to use geopackages. |
This provides options to return the alpha radius from
alpha_shape_auto
, as well as the detected bounding disks.It still needs
I keep with the rest of the package using
radius
to refer to the radius of the circles,alpha
to refer to1/radius
, andcircle
(notdisk
) to refer to the shapes defining thealpha_shape
. Strictly speaking, Edselbrunner et al usealpha
to meanradius
itself, since when alpha = infty, the alpha shape is the convex hull (the space bounded by circles with infinite radius). Two points are "alpha-neighbors" when they admit a circle of radius alpha with no other points. etc. But, since we're consistent, I think this is fine.In theory, I suppose this should be exposed for the individual
alpha_shape
method, but sincealpha
is specified in that case, it's not very hard to do: