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

Picking the first polygon/multipolygon found in graph_from_place() #320

Closed
Jukurrpa opened this issue Jul 30, 2019 · 6 comments · Fixed by #536
Closed

Picking the first polygon/multipolygon found in graph_from_place() #320

Jukurrpa opened this issue Jul 30, 2019 · 6 comments · Fixed by #536

Comments

@Jukurrpa
Copy link

Problem description (what did you do, what did you expect to happen, and what actually happened)

I have a script that uses osmnx's graph_from_place() to fetch and manipulate OSM data within the boundaries of cities. The script can potentially be run on virtually any city.

I've encountered issues for some cities where their administrative boundaries weren't the first result given by OSM's nominatim API, such as Toronto, Canada where the first result is the city's POI, causing the method to throw a ValueError as it's a point. Because I can't know this in advance, I need to have a loop that calls graph_from_place() with which_result=1, then 2, etc. until the boundaries are found. This works but is not super intuitive.

I was wondering if a new parameter could be added to tell graph_from_place() to use the first (multi)polygon it finds, with a maximum number of results to check.

As an example, osmnx.graph_from_place('Toronto, Canada', find_within_results=5) would get the five first results from OSM and use the first one that's a (multi)polygon. Actual name of the parameter to be defined of course :) This new parameter and which_result would be mutually exclusive. From reading the code it would mainly imply changing gdf_from_place() to accept this new parameter as well and use it as a limit for osm_polygon_download() and loop through the results.

If that sounds reasonable I'd be interested in implementing it.

@gboeing
Copy link
Owner

gboeing commented Oct 2, 2019

This is an interesting idea. I'm a bit apprehensive about it because automating the (multi)polygon search can open up the user to all kinds of unexpected behavior when they get a county or metro or even downtown definition of what they were expecting to be a municipality (as can happen in some places). The value of making which_result explicit in my mind is allowing (enforcing?) the user to identify the correct study boundary explicitly. The only other thing is that we'd want to propagate this functionality through to all the other functions that use polygons to query overpass (such as the footprints module and the pois module).

@Jukurrpa
Copy link
Author

Jukurrpa commented Oct 3, 2019

Got it, so in you actually want people to look up the results and figure out which one they need before calling graph_from_place(..., which_result=n)? Sounds reasonable as I've since had weird results when looking for cities sometimes, like the county coming up before the city even with the proper zoom level in the search. It's issues that would be very easily missed with my proposal.

When I first posted it it seemed like a convenience feature but its potential issues are not worth it I guess. Feel free to close the request!

@kenomaerz
Copy link

Thank you for redirecting me here, @gboeing!
I personally found the behavior a bit confusing, because I could not easily match the error message to a solution and felt that this could be resolved automatically. I see why an automatism may not be the right, choice, but I would suggest to be a bit more verbose, e.g. check the result type early, and if it is not a geometry, log a warning pointing to which_result as a solution.

Additionally, maybe one could add a magic "auto" or -1 value for which_result that actually does the auto-selection when specified. That would allow the user fine grained control without hiding complexity. I'd also offer to implement, but get in line behind @Jukurrpa ;)

@gboeing
Copy link
Owner

gboeing commented Jul 8, 2020

Thanks @kenomaerz

maybe one could add a magic "auto" or -1 value for which_result that actually does the auto-selection

If this feature gets implemented, I think this would be the right way to do it. What I might recommend though, would be to leave the current default argument which_result=1 but allow the user to optionally pass which_result=None. If None, then auto-select the first (multi)polygon in the GeoDataFrame.

The other thing that I mentioned in an earlier comment is that we'd want to propagate this functionality consistently through to all the other functions that use polygons to query overpass. Off the top of my head, this would include at a minimum the graph, geocoder, footprints, and pois modules. That would allow all querying to be specified in a consistent manner.

I'm going to reopen this issue because I think I'm open to this implementation if you'd like to take a crack at it.

if it is not a geometry, log a warning pointing to which_result as a solution

Note that OSMnx does currently log a warning if the geometry is not a (multi)polygon. This warning does not explicitly point the user to the which_result parameter though.

@gboeing gboeing reopened this Jul 8, 2020
@kenomaerz
Copy link

That all sounds reasonable to me, I'll take a stab at it. I cannot promise any timelines right now, but let you know once I have some progress. I do not yet have an overview over the other functions that need a similar treatment - I'll look at the modules you named, but the solution will probably require some review in regards to what else needs attention.

@gboeing
Copy link
Owner

gboeing commented Jul 21, 2020

I have taken a pass at this today in PR #536. @Jukurrpa @kenomaerz if you'd both be so kind as to review and test that PR to see if it works as expected with a variety of queries, I'd be grateful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants