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

Opq parameter to select a set of OSM types #295

Merged
merged 11 commits into from
Jan 19, 2023
Merged

Conversation

jmaspons
Copy link
Collaborator

Closes #257 (and the duplicated #293)

@jmaspons
Copy link
Collaborator Author

Draft until PR #291

@mpadge mpadge mentioned this pull request Dec 19, 2022
6 tasks
@jmaspons jmaspons force-pushed the opq-types branch 2 times, most recently from 6f47d67 to 97d0d41 Compare December 19, 2022 16:31
R/opq.R Outdated Show resolved Hide resolved
@jmaspons jmaspons marked this pull request as ready for review December 19, 2022 16:36
@jmaspons
Copy link
Collaborator Author

Last commit is not really related but it fix opq_osm_id wich I discovered while using the package

@mpadge
Copy link
Member

mpadge commented Dec 30, 2022

Thanks @jmaspons for all this great work. I'll wait for #298 now, and then progress to this one.

@jmaspons
Copy link
Collaborator Author

Rebased and ready for review @mpadge

@mpadge
Copy link
Member

mpadge commented Jan 18, 2023

That looks great @jmaspons, thanks again for the great work. I just have one question: What is the intention of exposing "area" as an osm_type? The ability to do this is really cool, but as far as i understand it, it's only really useful when used as a pivot to filter nominated types (= osm_type) within a specified area. Passing area alone without a pivot directly returns all identified "area" elements within the nominated (bounding) area. These are mostly multipolygons, but are actually structured in the XML as <area> elements, which the code here does not know what to do with. All functions still work in these cases, but the <area> elements, and thus all multipolygons and most polygons, simply get ignored.

Test code:

opq (bbox = c (-0.118, 51.514, -0.115, 51.517), osm_types = "area") |>
    add_osm_feature (key = "highway") |>
    osmdata_xml (q, filename = "junk.osm", quiet = FALSE)

There are all kinds of overpass XML elements that are possible, but for which this package will have to adapted in the future, if and when anybody requests any features like that. I think that "area" in this case might be best just left out of acceptable osm_types, because it can't really be used as is, and filtering by area is already demonstrated nicely in your example code you've added to opq(). What do you think?

These elements are returned as as <area> elements in XML, which the osmdata
code does not know what to do with. All functions still work in these
cases, but the <area> elements, and thus all multipolygons and most
polygons, simply get ignored.
@jmaspons
Copy link
Collaborator Author

Ok, area type removed. I just included all possible values in overpass, but I don't have a use case for this. If somebody makes the request, we can think about that later.

@jmaspons
Copy link
Collaborator Author

test-coverage checks failed 2 times in a row. Is there any reason or just a random thing?

@mpadge
Copy link
Member

mpadge commented Jan 18, 2023

Thanks! The test failures are not random. If you click open a couple of layers deep, you get to this:

── Failure ('test-osmdata.R:243'): make_query ──────────────────────────────────
Names of attributes(res) ('names', 'row.names', 'class', 'bbox', 'overpass_call', 'meta') don't match 'names', 'class', 'row.names', 'bbox', 'overpass_call', 'meta'

Can you just tweak that test to expect_named (..., ignore.order = TRUE)? Then good to merge.

@jmaspons
Copy link
Collaborator Author

I would expect that also all R-CMD-check fails 🤷‍♂️. Should be related to #298. I will reorder names as I don't expect to change quite often

@mpadge mpadge merged commit accb301 into ropensci:main Jan 19, 2023
@jmaspons jmaspons deleted the opq-types branch February 2, 2023 10:01
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.

[FEATURE] Limit queries to specific feature types
2 participants