ropensci / osmdata

R package for downloading OpenStreetMap data
https://docs.ropensci.org/osmdata
314 stars 45 forks source link

Opq parameter to select a set of OSM types #295

Closed jmaspons closed 1 year ago

jmaspons commented 1 year ago

Closes #257 (and the duplicated #293)

jmaspons commented 1 year ago

Draft until PR #291

jmaspons commented 1 year ago

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

mpadge commented 1 year ago

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

jmaspons commented 1 year ago

Rebased and ready for review @mpadge

mpadge commented 1 year ago

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?

jmaspons commented 1 year ago

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 commented 1 year ago

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

mpadge commented 1 year ago

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 commented 1 year ago

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