Add oe_get_network #218
Add oe_get_network #218
Conversation
|
It sounds good. An alternative would be |
|
@Robinlovelace can you check the options set for the three modes of transport? If those are ok, I just need to document the function. |
There was a problem hiding this comment.
This is a great start.
Minor comments:
- In terms of documentation it's worth mentioning and adding a link to the source for the filters
- One for later: document how to create a custom network creation function, e.g.
net_wheelchair() - Mixed styles, the package has
=and<-assignment on this branch
These are minor points. Overall big 👍
R/get-transport.R
Outdated
| 'planned', 'platform', 'proposed', 'raceway' | ||
| )) | ||
| AND | ||
| (foot IS NULL OR foot != 'no') |
There was a problem hiding this comment.
I was going to sat that motorways cannot be cycled on with reference to https://wiki.openstreetmap.org/wiki/Tag:highway%3Dmotorway
But then I read there that highway=motorway implies that bicycle=no.
Do we no if that tag gets auto tagged?
There was a problem hiding this comment.
Looking at a motorway in Leeds that you definitely cannot cycle on I don't think it does get auto added: https://www.openstreetmap.org/way/400349319
| 'fixme', 'escalator', 'gallop', 'historic', 'no', 'planned', 'platform', | ||
| 'proposed', 'raceway', 'steps' | ||
| )) OR ( | ||
| highway IN ('motorway', 'motorway_link', 'footway', |
There was a problem hiding this comment.
OK I see motorway is excluded, great.
There was a problem hiding this comment.
Motorways and motorway_links are always excluded unless the same OSM way has "bicycle = yes" (as documented here)
|
Thanks for the comments! Tomorrow I will fix the docs. |
There was a problem hiding this comment.
Overall great PR, very useful new functionality and ready to ship now 🚢
My comments are minor and can be tidied up post merge - feel free to change any of my commits to this branch also @agila5. Thanks for great work on this 🎉
| @@ -25,7 +25,7 @@ jobs: | |||
| matrix: | |||
| config: | |||
| - {os: windows-latest, r: 'release'} | |||
| - {os: macOS-latest, r: 'release'} | |||
| # - {os: macOS-latest, r: 'release'} | |||
There was a problem hiding this comment.
FYI this can be re-added if you add the lines shown here to the .yaml file:
I suggest this is a separate issue, just mentioning here as an FYI.
| #' @export | ||
| #' | ||
| #' @details The `cycling` mode of transport (i.e. the default value for `mode` | ||
| #' parameter) selects the OSM ways that meet the following conditions: |
There was a problem hiding this comment.
Great to have these details.
| #' @return An `sf` object. | ||
| #' @export | ||
| #' | ||
| #' @details The definition of usable transport network was taken from |
There was a problem hiding this comment.
Is this correct @agila5 ? If we modify the rules we could say 'was inspired by' but as far as I'm aware this is currently like-for-like.
There was a problem hiding this comment.
It was not exactly "taken" from osmnx but I will slightly change that in master branch, 👍
| #' | ||
| #' @seealso [`oe_get()`] | ||
| #' | ||
| #' @examples |
| oe_get_options = check_args_network(dots_args, oe_get_options) | ||
|
|
||
| # Run oe_get | ||
| do.call(oe_get, oe_get_options) |
There was a problem hiding this comment.
Clever! I learned something from this, I think, all the named args are passed here, right?
There was a problem hiding this comment.
Yup! Thanks also to @luukvdmeer and @loreabad6, I learned that when reading the code behind plot.sfnetwork 😅
|
|
||
| # The following functions are used to load several ad-hoc vectortranslate | ||
| # options according to a specific mode of transport. The choices are documented | ||
| # in oe_get_network() and are based on the following documents: |
There was a problem hiding this comment.
Big 👍 for including these links, may make my previous comment about osmnx redundant.
Just the first ideas. Will need to fix the filters + several intermediate checks.
@Robinlovelace what do you think about the name of the function?