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

Allow vectors of ids + types for opq_osm_id #283

Merged
merged 7 commits into from
Nov 29, 2022

Conversation

jmaspons
Copy link
Collaborator

Requires #282

@mpadge
Copy link
Member

mpadge commented Nov 28, 2022

Thanks @jmaspons, once you've updated tests for #282, i'll merge that and get straight on to this one.

@mpadge
Copy link
Member

mpadge commented Nov 28, 2022

@jmaspons I see what you're trying to do here, and think that this also is a good improvement. It still needs a bit of work though. Overpass queries should have all of the type entries grouped together, whereas your current line here:

id <- paste(sprintf (" %s(id:%s);\n", opq$id$type, opq$id$id), collapse="")

Just puts every ID with its own distinct type, so submits queries like (node(id:1); node(id:2); node(id:3)). It's more efficient, and thus necessary here, to group these together, to give (node(id:1,2,3)). One way to do that would be to replace the line you have above (L#809 fof opq.R) with something like:

if (!is.null (opq$id)) { # opq with opq_osm_id

    id_types <- data.frame (type = opq$id$type, id = opq$id$id)
    id_types <- lapply (split (id_types, f = factor (id_types$type)), function (i) {
        sprintf (" %s(id:%s);\n", i$type [1], paste0 (i$id, collapse = ","))
    })
    id_types <- paste0 (id_types, collapse = "")
        
    res <- paste0 (opq$prefix, id_types, opq$suffix)

}

Note that split groups things regardless of their original order, so even if you submit "way", "rel", "way", "rel", they'll still just be split into the appropriate two groups.


Another minor issue is that the initial checks also need to be improved. You've currently just added all to the length(id) == 1 && grepl(...) bit, but that's not quite sufficient here. Those checks need to be replaced with checks to ensure that either length(type) == 1 or length(type) == length(id), and then proceed to ensure all type values are the 3 allowed.


I think those two tweaks should pretty much get us there - let me know when that's done, and i'll aim to merge asap after that. Thanks!

@mpadge
Copy link
Member

mpadge commented Nov 28, 2022

One more thing: There will once again also be failing tests which will need to be updated as well. You can run those locally yourself (in test-opq.R) to see what they are, and fix where necessary.

@jmaspons
Copy link
Collaborator Author

I think it's ready.

For the initial checks, I removed the length(id) == 1 as since #282 opq can work with multiple ids, now also for id in type/id format.

Also, instead of testing length(type) == length(id), I test for length (id) %% length (type) != 0, a condition for recycling when creating the data.frame in opq_string

Copy link
Member

@mpadge mpadge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Those edits are just to conform with camel case standard here - no upper case in variable names.

R/opq.R Outdated Show resolved Hide resolved
R/opq.R Outdated Show resolved Hide resolved
R/opq.R Outdated Show resolved Hide resolved
@jmaspons
Copy link
Collaborator Author

snake_case done!

@mpadge mpadge merged commit 2605e00 into ropensci:main Nov 29, 2022
@jmaspons jmaspons deleted the opq_osm_id-vectors branch November 29, 2022 14:44
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.

2 participants