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

Unions filter #1199

Closed
3 tasks done
arkty opened this issue Aug 6, 2024 · 5 comments
Closed
3 tasks done

Unions filter #1199

arkty opened this issue Aug 6, 2024 · 5 comments

Comments

@arkty
Copy link
Contributor

arkty commented Aug 6, 2024

Contributing guidelines

  • I understand the contributing guidelines

Documentation

  • My proposal is not addressed by the documentation or examples

Existing issues

  • Nothing similar appears in an existing issue

What problem does your feature proposal solve?

When developing city graph models, the size of the resulting graph might be important. Thanks to OSMnx, it's possible to use a pre-defined filters to request a network, based on desired types of OSM ways. But sometimes, it's needed to add an additional ways from OSM into network or union them based on multiple filters.

What is your proposed solution?

Make custom_filter accepts not only string, but a list of filters and then combine the results into single graph.
Because OSMnx already supports sub-divided requests, it's easy to implement this without changing logic on actual graph creation (simplification, connectivity, etc).

What alternatives have you considered?

Requesting osmnx.graph_from_* multiple time and then merging result graphs manually without simplification.

Additional context

The idea is described in this PR.

@gboeing
Copy link
Owner

gboeing commented Aug 6, 2024

Interesting idea for a relatively minimal change to make this a bit smoother for the user. I'm trying to think through the implications and trade-offs of this technique. Please share any insights as such.

See also the discussion at gboeing/osmnx-examples#90 for another (possibly infeasible) idea of addressing this.

@arkty
Copy link
Contributor Author

arkty commented Aug 7, 2024

Another way to implement this is actually use overpass unions.
This will require to change query creation.

Example code (not tested).

    for polygon_coord_str in polygon_coord_strs:
        way_subquery_str = ""
        for way_filter_str in way_filter:
            way_subquery_str += f"(way{way_filter_str}(poly:{polygon_coord_str!r});"
        query_str = f"{overpass_settings};{way_subquery_str}>;);out;"
        yield _overpass_request(OrderedDict(data=query_str))

In some cases, polygon_coord_str can be really long (because poly statement will appear in each way query), therefore request payload size will be increased. I have not found any limits about request size on Overpass side, so this still can be an implementation option.


Adding support for custom_filter to be a list of filters looks like a simple change, but I am not sure will this confuse users or not, because it might be not clear that custom_filter: list[str] means union filter.

On the other hand, creating own query structure for custom_filter will lead to reproducing Overpass query language. And this not looks like a thing that can be easily developed and maintained.

@gboeing
Copy link
Owner

gboeing commented Aug 12, 2024

Overall I think making this change to custom_filter: str | list[str] | None = None would be a sensible enhancement to simplify union queries (thus avoiding nx.compose).

@arkty would you like to open a PR?

@EwoutH this may be of interest given your question at the examples repo.

@arkty arkty mentioned this issue Aug 13, 2024
@arkty
Copy link
Contributor Author

arkty commented Aug 13, 2024

@gboeing i've created a PR #1204.

@gboeing
Copy link
Owner

gboeing commented Aug 14, 2024

Closed by #1204

@gboeing gboeing closed this as completed Aug 14, 2024
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

No branches or pull requests

2 participants