Add minimal R package (#67) #68
Conversation
r/DESCRIPTION
Outdated
| Depends: | ||
| R (>= 2.10) | ||
| LazyData: true | ||
| URL: https://acteng.github.io/od2net/ |
There was a problem hiding this comment.
od2net.org/r?
There was a problem hiding this comment.
Is this the repo URL or where the generated documentation will live? https://urban-analytics-technology-platform.github.io/od2net/r/ for the docs; I don't have path forwarding configured on od2net.org yet
There was a problem hiding this comment.
Is this the repo URL or where the generated documentation will live? https://urban-analytics-technology-platform.github.io/od2net/r/ for the docs; I don't have path forwarding configured on od2net.org yet
Correct. Sounds good.
There was a problem hiding this comment.
I think I'm confused about the purpose of a common library in R here. https://github.com/Urban-Analytics-Technology-Platform/od2net/blob/main/examples/utils.py is the Python equivalent so far. The only really interesting method in there is for turning an OSM file into building centroids. The R port has that, and also has a useful helper for downloading + clipping OSM based on the bbox of the zones file. The rest of it all feels like stuff specific to one example. It's making big assumptions about the zone / OD input files. That feels like an example use of the library, not something that belongs in the library, right?
r/man/make_elevation.Rd
Outdated
There was a problem hiding this comment.
Is it normal to check in generated files?
There was a problem hiding this comment.
Yes, they are needed for the R tests to pass.
r/R/setup.R
Outdated
| sf::write_sf(centroids, "input/buildings.geojson", delete_dsn = TRUE) | ||
| } | ||
|
|
||
| make_od = function() { |
There was a problem hiding this comment.
Why is this hardcoding one particular file? This feels like example code, not something that belongs in a library
There was a problem hiding this comment.
Agreed, should be a user-provided file.
r/DESCRIPTION
Outdated
| Depends: | ||
| R (>= 2.10) | ||
| LazyData: true | ||
| URL: https://acteng.github.io/od2net/ |
There was a problem hiding this comment.
Is this the repo URL or where the generated documentation will live? https://urban-analytics-technology-platform.github.io/od2net/r/ for the docs; I don't have path forwarding configured on od2net.org yet
r/NAMESPACE
Outdated
There was a problem hiding this comment.
Why is a generated file checked in?
There was a problem hiding this comment.
This is needed for the R package.
There was a problem hiding this comment.
Standard practice to check-in this small plaintext file, e.g.: https://github.com/duckdb/duckdb-r/blob/main/NAMESPACE
r/R/setup.R
Outdated
| #' It will generate a file in the input/ folder | ||
| #' | ||
| #' @param file Location or URL of zones file | ||
| make_zones = function(file) { |
There was a problem hiding this comment.
I don't understand what this function actually does that's helpful. A user needs to make a GJ file with zones and have a name property on each one. This reads a file, arbitrarily renames one of the properties to name, and writes? That feels very specific / making assumptions about the input
There was a problem hiding this comment.
It assumes that the first column is the name, which is the case with most zone datasets, needs documentation for sure if we do include it.
There was a problem hiding this comment.
Removed now.
r/R/setup.R
Outdated
| sf::write_sf(zones, "input/zones.geojson", delete_dsn = TRUE) | ||
| } | ||
|
|
||
| getbbox_from_zones = function() { |
There was a problem hiding this comment.
This and make_osm do feel like useful functions to be in a common library. They don't make many assumptions about what the user is trying to do.
r/R/setup.R
Outdated
| #' @param file File name if hosted on a known site | ||
| #' @param base_url Base URL associated with the 'file' argument | ||
| #' | ||
| make_elevation = function( |
There was a problem hiding this comment.
This also feels too specific to be useful, or it's not really doing much. The user needs to find a geotiff in the correct format. All this function does is download a file and gunzip it. Does it belong in the example and not the common library?
There was a problem hiding this comment.
Yeah. Can remove or move to tutorial...
r/R/setup.R
Outdated
| } | ||
| } | ||
|
|
||
| make_origins = function() { |
There was a problem hiding this comment.
I would call this make_building_centroids or something, and maybe in the docs, call out the caveats with that approach. OSM building coverage is very spotty, and that'll very much affect the results of od2net. That's something the user needs to think about
There was a problem hiding this comment.
Yes, a lot of this can be in the docs, documentation may be more important than the actual functions here.
r/R/setup.R
Outdated
| readr::write_csv(od, "input/od.csv") | ||
| } | ||
|
|
||
| main = function() { |
There was a problem hiding this comment.
Same here, does this belong in a library?
There was a problem hiding this comment.
No, will remove.
|
Thanks for the detailed review @dabreegster, very useful and will improve the experience for R users wanting to use |
|
Yep, sounds good. I think that's a pattern I've seen -- the library has helpers likely to be widely useful, and the end-to-end example can do a bunch of specific stuff |
|
Checks passing:
|
No description provided.