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

shapely backend for isochrone #398

Merged
merged 9 commits into from
Mar 4, 2024
Merged

shapely backend for isochrone #398

merged 9 commits into from
Mar 4, 2024

Conversation

knaaptime
Copy link
Member

@knaaptime knaaptime commented Mar 2, 2024

  • isochrones can use libpysal or shapely
  • fast network downloader based on osmnx
  • new example notebook including auto-based isos
  • test on apple silicon
  • fix for coverage

this allows the use of concave hull from shapely in addition to alpha shapes from libpysal for isochrone generation. The concave hull is faster, and can be much tighter. Thats not a very fair comparison since we're using 'auto' alpha shapes, but the shapely's hull seems easier to tune than the manual auto shape (good results around ratio=0.2)

also resolves #399

@knaaptime knaaptime changed the title lodescodes concave backend for isochrone Mar 2, 2024
@knaaptime knaaptime requested a review from sjsrey March 2, 2024 08:17
@knaaptime knaaptime added the enhancement New feature or request label Mar 2, 2024
@knaaptime
Copy link
Member Author

i'm not in love with the algorithm={'alpha','hull'}, terminology since an alpha shape is one type of concave hull. Maybe it would be better to use backend={'libpysal', 'shapely'}?

@knaaptime knaaptime requested a review from jGaboardi March 2, 2024 22:59
geosnap/analyze/network.py Outdated Show resolved Hide resolved
geosnap/analyze/network.py Show resolved Hide resolved
geosnap/analyze/network.py Outdated Show resolved Hide resolved
geosnap/analyze/network.py Show resolved Hide resolved
geosnap/analyze/network.py Outdated Show resolved Hide resolved
@knaaptime
Copy link
Member Author

@ljwolf two quick questions if you have a minute:

  1. being more engaged with the cg literature, do you have an opinion on the algorithm/backend/hull parameter discussion? At the moment I'm partial to hull as an argument with options for shapely and libpysal
  2. slightly off-topic, but one thing ive used this adj builder for recently is dumping quickly (and nicely) into a Graph, which works at the moment, but only because the Network is denser than the actual observations that get snapped to it. Once there are duplicate observations at each network node, we'll need something like the clique expander... do you think that would be easy to hook up here?

@knaaptime knaaptime changed the title concave backend for isochrone shapely backend for isochrone Mar 3, 2024
@knaaptime knaaptime merged commit dcf3b27 into main Mar 4, 2024
6 checks passed
@knaaptime knaaptime deleted the lodescodes branch March 4, 2024 16:51
@ljwolf
Copy link
Contributor

ljwolf commented Mar 4, 2024

On 1, I think I would prefer to deprecate the libpysal.cg.alpha_shape functionality 😅 Now that it's present in shapely, I think we should move away from it for further implementation. The one nice thing about our implementation is that it can construct the binding circles, but that's not enough to really use it over shapely's.

On 2, yes, you'd need to convert the dict(zip()) to build a proper one OSMID-to-many OriginalID lookup table. Then, the clique expander would use that with the OSMID adjacency to create the OriginalID-to-ID adjacency.

I think the current implementation will silently drop any points if they are snapped to the same OSMID:

import numpy

# many origins have the same OSMID
osmids = list(numpy.arange(5).astype(str))*3
origins = numpy.arange(15)

# but only the last one enters the lookup
dict(zip(osmids, origins))

# what _induce_cliques() needs is like:
pandas.DataFrame.from_dict(
    dict(osm=osmids, origins=origins)
).groupby("osm").origins.agg(list).to_dict()

@ljwolf
Copy link
Contributor

ljwolf commented Mar 4, 2024

Oh, and _induce_cliques() should be able to correctly rename an input adjacency table even if there's a one-to-one mapping!

@knaaptime knaaptime mentioned this pull request Mar 4, 2024
@knaaptime
Copy link
Member Author

@ljwolf dope, thank you. that's straightforward

(on 1, i made shapely the default with ratio=0.2, which works nicely for this application. On the binding circles, I we should be able to grab that from geos now? though not sure exactly which options are exposed in shapely. Doubt its worth reimplementing either)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

coverage reported incorrectly
4 participants