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

Adding overpass_trim() for filtering by area in overpass #258

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Mashin6
Copy link
Contributor

@Mashin6 Mashin6 commented Dec 11, 2021

This PR addresses suggestion #256.

Main changes are:

  1. New function added overpass_trim() that modifies OP query to subset OSM features by area rather than currently default bbox. Function parses entered information about areas to be used for trimming and stores them in a list in overpass_query object: trim_area = list (ways = c(ID, ... ), relations = c(ID, ...)
  2. opq_string_intern() was modified to accommodate changes necessary for area filtering.
  3. A new variable for feature type ftype was introduced in opq_string_intern that allows easy switching of which object types are queried. This can be easily extended to more types when [FEATURE] Limit queries to specific feature types #257 would be implemented.
  4. OP queries for nodes, ways, relations are not as 3 separate rows, but as a single nwr[...];
  5. WARNING: The most controversial change could be that I removed checks for if (is.null (bbox)) from functions: opq(), add_osm_feature(), add_osm_features() and introduced final check at the beginning of opq_string_intern. This was to allow opq() to be called without getbb(). So the queries can be now built like this:
opq() |>
        add_osm_feature(key = "natural",
                        value = "tree") |>
        overpass_trim(id = c(11597767, 43437030), 
                      type = c("relation", "way") ) |>
        osmdata_sf()

or

a <- getbb("portsmouth usa", format_out = "data.frame")
opq() |>
        add_osm_feature(key = "amenity",
                        value = "restaurant") |>
        overpass_trim(osm_area = a) |>
        osmdata_sf()

The only thing I am not happy about is that a typical user would expect trimming to work like this instead:

opq("portsmouth usa") |>
        add_osm_feature(key = "amenity",
                        value = "restaurant") |>
        overpass_trim() |>
        osmdata_sf()

But that would require making bbox_to_string return object id and type along with its bbox and let opq sort it out into $bbox and $trim_area. Any ideas?

@Mashin6 Mashin6 marked this pull request as draft January 5, 2022 01:01
@jmaspons
Copy link
Collaborator

Are you still open to work on this? I'm interested in the functionality and I can help in the implementation. Any feedback from the contributors would be useful.

@mpadge
Copy link
Member

mpadge commented Nov 29, 2022

@jmaspons yes, i am very keen for something to happen with this - it's already been open way too long, which ain't good 😱 I should also be able to allocate some time to this soon too, but please feel free to see what you can do in the meantime. It's an important idea.

@Mashin6
Copy link
Contributor Author

Mashin6 commented Nov 29, 2022

@mpadge Would you be able to provide any feedback on the code/changes proposed? There were deeper modifications in the underlaying code and would be good to know your opinion if this is even in the right direction.

@mpadge
Copy link
Member

mpadge commented Nov 29, 2022

Yeah, for sure @Mashin6, and great that you responded so promptly after such a long time - thank you! I'll get back to asap - hopefully in a few days when i find some time to look through it. Thanks!

@Mashin6
Copy link
Contributor Author

Mashin6 commented Nov 29, 2022

@jmaspons Did you have some ideas for implementing the overpass-side area trimming?

@jmaspons
Copy link
Collaborator

My idea is to pass a string in the form rel(id) to bbox parameter, which I would rename to area at some point. I have a draft jmaspons@cceab6f

Then, a get_area function, similar to getbb for areas, would help to get the string. If you like the idea, I can test the code and implement the get_area function and all de details.

@Mashin6
Copy link
Contributor Author

Mashin6 commented Nov 30, 2022

I like the simplicity of it. Would definitely like to see how it turns out.

@mpadge
Copy link
Member

mpadge commented Nov 30, 2022

@Mashin6 There've been a few changes to opq.R recently. Would you please be able to merge those within your branch, and then push the updated version to this PR? Thanks!

@jmaspons
Copy link
Collaborator

jmaspons commented Dec 5, 2022

I like the simplicity of it. Would definitely like to see how it turns out.

You can find a working implementation at #286

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.

3 participants