Review for ropensci - Githubissues

temospena commented 3 years ago

[x] open a parallel branch for revs
[x] update readme: include more on the description? the main functions?

The README has be substantially updated and now documents the package's main functions: https://github.com/ITSLeeds/slopes/commit/dbacab8792dfc178c37b3ceb5aafd9791f515061

[x] pass benchmark to a vignete
[x] Update argument names
[x] Don't export plot_dz

More detailed responses in-line

Draft response to review 2:

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
* **Briefly describe any working relationship you have (had) with the package authors.**

* [x]  As the reviewer I confirm that there are no [conflicts of interest](https://devguide.ropensci.org/policies.html#coi) for me to review this work (If you are unsure whether you are in conflict, please speak to your editor _before_ starting your review).
Documentation

The package includes all the following forms of documentation:
* [x]  **A statement of need** clearly stating problems the software is designed to solve and its target audience in README

* [x]  **Installation instructions:** for the development version of package and any non-standard dependencies in README

* [ ]  **Vignette(s)** demonstrating major functionality that runs successfully locally

* [x]  **Function Documentation:** for all exported functions

* [ ]  **Examples** (that run successfully locally) for all exported functions

* [ ]  **Community guidelines** including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with `URL`, `BugReports` and `Maintainer` (which may be autogenerated via `Authors@R`).
Functionality
* [x]  **Installation:** Installation succeeds as documented.

* [x]  **Functionality:** Any functional claims of the software been confirmed.

* [x]  **Performance:** Any performance claims of the software been confirmed.

* [x]  **Automated tests:** Unit tests cover essential functions of the package and a reasonable range of inputs and conditions. All tests pass on the local machine.

* [ ]  **Packaging guidelines:**: The package conforms to the rOpenSci packaging guidelines
Estimated hours spent reviewing: 8-10
* [x]  Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.
Review Comments

This is a great package that fills an obvious need. It has all of the necessary functionality required to do the job it sets out to do - calculate slopes of routes. The functions for getting and using the appropriate DEM for an area removes a huge barrier for users, so kudos! I had fun test-driving the package and learned a lot. I hope my review doesn't come across as negative - it really is an excellent package and with some rough edges filed down I think it could be even better! I did get a bit in the weeds at times - so while there are a lot of comments most of them are quite small!

Review Checklist

Documentation

Statement of Need

The introductory sentence in the README gives a very brief description of what the package does, and the target audience is fairly easily inferred (i.e., people who need to calculate slopes of linear features). I don't think this needs much more as the application is fairly clear, however you could include an overview of some of the other things the package does to help a user get to the point of calculating slopes (e.g., getting elevation data from online sources, extracting elevation points), as well as the visualization tools (plotting functions). I like that it compares the package's functionality to other common tools (ESRI's Slope3d()).

[x] Installation instructions

* Installation instructions work and are straightforward, however it would be helpful to add some information about MapBox, and that an API key is needed for some functions. Also note that for some functions, there are packages that will need to be installed separately as they are not installed automatically (i.e., are in `Suggests` - `ceramic`, `ggdist`, etc.)

[x] Vignette(s)

* The main vignette (`slopes.Rmd`) doesn't actually show any examples of what the stated main purpose of the package is - to provide slopes of linear features from a DEM. The README has the most comprehensive walkthrough of the main advertised functionality of the package. It is available on GitHub and the pkgdown page - but not available in R as a vignette. I suggest taking the README content and creating a vignette (likely the primary one), or perhaps even better - add it to the current `slopes.Rmd` so you combine the theory with a demonstration of the implementation.

* `verification.Rmd`:

  * a `library(raster)` call is needed to plot the raster object (`plot(e)` on [line 91](https://github.com/ITSLeeds/slopes/blob/3a25f7ce11ced3be313c0eadbda395551887fa2e/vignettes/verification.Rmd#L91))
  * `slope_raster(r, e = e)` on [line 123](https://github.com/ITSLeeds/slopes/blob/3a25f7ce11ced3be313c0eadbda395551887fa2e/vignettes/verification.Rmd#L123) fails because `points2line_trajectory()` does not assign CRS to the output - as a result, `lonlat` is not determined internally (more on this below). I made it work here by tweaking `points2line_trajectory()`:

  ```
  points2line_trajectory = function(p) {
    c = st_coordinates(p)
    i = seq(nrow(p) - 2)
    l = purrr::map(i, ~ sf::st_linestring(c[.x:(.x + 1), ]))
    lfc = sf::st_sfc(l)
    a = seq(length(lfc)) + 1 # sequence to subset
    p_data = cbind(sf::st_set_geometry(p[a, ], NULL))
    sf::st_sf(p_data, geometry = lfc, crs = st_crs(p)) # set the crs here
  }
  ```

We have made substantial improvements to the vignettes. There is now an updated introductory slopes.Rmd vignette that demonstrates all key function and a shorter README that links to the other vignettes. The more theoretical introduction to slopes has been moved into a new vignette, intro-to-slopes. See here for details: https://github.com/ITSLeeds/slopes/pull/38

Function Documentation

General Comments:

[x] Package-level documentation.

* Package-level documentation would be convenient, so a user can type `?slopes` and get an overview of what the package does, authors, urls etc. - and link to the package index). You can set this up with `usethis::use_package_doc()`.

This has been done in https://github.com/ITSLeeds/slopes/commit/018d2d60d0d8065d9d5865c98752b34cd14806bf. Many thanks for the tip.

[x] @return sections

* All function documentation should have a `@value` (return) section so the user knows what they can expect from the function.

[x] \value{...} sections have been added to the .Rd files using #' @return in the source code thanks to roxygen2. See commit https://github.com/ITSLeeds/slopes/commit/7a8cdc519828aae10ca7d4d4bf3e20a2ca11683f
[x] Datasets

* The included datasets are great - nice tidy real-world examples to play with.

[x] Dataset docs

* The datasets could be documented a bit more clearly - how many road segments, lengths/distances etc. It's nice to have the context when you are playing with the examples. It is not obvious to the user the differences between the various datasets with Lisbon road segments.

This is a good point, we put the dataset docs together in a rush. They have been iteratively improved, including in this commit: 45068459072a8602985e22e5d7730d24122a1cdd

[x] Argument names

* In a lot of cases the names of function arguments could be clearer - many are single letters that don't have an obvious meaning.

Thanks for the comment. We agree that the argument names were at times overly terse and we now find ourselves wishing we spent more time thinking about this earlier on. The good news is that, after some initial issues, we have now got updated and consolidated argument names, as documented here: https://github.com/ITSLeeds/slopes/pull/31/files. To summarise that we now have:

@param routes Routes, the gradients of which are to be calculated. replacing r
@param dem Raster overlapping withroutesand values representing elevations replacing e
@param elevations Elevations in same units as x (assumed to be metres) also replacing e

We also updated arg names in the plotting functions in https://github.com/ITSLeeds/slopes/commit/7651276edab847dd0ce27d33ed182669ae3b28a9

[x] Updated description: 311af41fcced0157ab7e311008fef8b2570d20c8

* Data documentation should provide clearer sources for the data - for example, `dem_lisbon_raster` has only a link to a `terra` GitHub issue. The various Lisbon road segment datasets link to the ESRI's 3D Analyst extension webpage - which is not the source of the data, rather the tool that created it.

[x] Documentation for elevation_get()

Specific documentation issues:

* `elevations_get()` could use more explanation of how these elevations are obtained and how to customize it. What web service(s) is it using? I see that it uses MapBox via `ceramic::cc_elevation()` however that might be a bit opaque to a user. I think at least a mention that they need a MapBox API key (ideally with a link/instructions on how to get one), and a link to the `ceramic::cc_elevation()` documentation would be helpful, as well as some examples showing customization by passing options to `cc_elevation()`.

The documentation of this function has been improved, as can be seen in https://itsleeds.github.io/slopes/reference/elevation_get.html. Specifically, we link to long form documentation explaining how to get an API key, rather than duplicating content in the function documentation. We also link explicitly to the cc_elevation() page for people who want more details. Note: we do not see this functionality, that is reliant on potentially temperamental web APIs, as core to the package so we do not want to emphasise this aspect of the package, believing that it's better if users get and understand their own DEM data using other tools, in line with the Linux philosophy of modularity. See https://github.com/ITSLeeds/slopes/commit/7a67e2db3caeccf42c693067076ce224cbaf2de1 for details on updated links to the ceramic package.

[x] elevation_extract() documentation

* `elevation_extract()` documentation would benefit from more explanation of its arguments: what other options can be used for `method`? What are the benefits of using `terra` (and what is the alternative? - I know it's `raster` but the user may not).

The documentation has been improved as a result of this comment. See https://github.com/ITSLeeds/slopes/commit/18b0a5dbd7626b0b05029bfcd298bde38fd16796 for details.

One question for the reviewers: should this function even be exported? It seems of little additional use compared with the original extract() functions for users but we thought it worth asking and minimising changes. If nothing else the documentation is now more interesting!

[x] plot_dz

* `plot_dz`:

  * All argument documentation should be expanded, at a minimum stating the defaults, the accepted options (and/or type/format of inputs).

[x] Agreed. To ease maintenance, @param tags have been shifted up into the exported function plot_slope(). All parameters are now better documented, with improved names (legend_position instead of x, for example) and default values.

  * I tend to think that the [setting of the default colour](https://github.com/ITSLeeds/slopes/blob/9365709a95312b4feff3689a9e219c06af314167/R/plot_slope.R#L63-L66) palette (`p`) could be done internally in the function, and set the default argument value to `NULL` - this would be a simpler interface for the user. In addition, the value of `requireNamespace()` does not need to be assigned to anything in this `ifelse()` statement.

[x] The interface has been simplified and we removed the ifelse statement. The parameter is still set with a default but now that colorspace is imported it is guaranteed to work.

  * It is not clear (to me at least) what the argument `x` does - I can infer that it is legend position, but neither the name of the argument nor the description tell you this. I suggest renaming it and explaining what it does.

[x] plot_slope() arguments

  * It's not clear what the values passed to `s` do, nor what significance the default values `3:18` hold (especially as they relate to `brks`).

The parameter name has been clarified and renamed:

#' @param seq_brks Sequence of breaks to show in legend.
#'   Includes negative numbers and omits zero by default

[x] plot_slope() changes

* `plot_slope()` - Similar comments on argument names, defaults and descriptions as `plot_dz`. It seems to me that this is the function that users would call much more often than manually constructing the components and then using `plot_dz()` - if that's the case, would you consider not exporting `plot_dz`, or directing users to `plot_slope()` in most cases?

Agreed, the plot_slope() function is the one that people will use and it was confusing having both plot_slope() and the more esoteric helper function plot_dz exported. Only plot_slope() is now exported and the documentation is better. Also, the colourscheme has been updated so 0 corresponds with white, which was not previously the case.

[x] sequential_dist()

* `sequential_dist()` - Some more clarity on what `m` should look like would be helpful. What are the dimensions of the matrix, what are the order of the columns? The example doesn't give much clarity on this.

The description of the input m has been updated and now provides this important information:

#' @param m Matrix containing coordinates and elevations.
#'   The matrix should have three columns: x, y, and z. Typically
#'   these correspond to location in the West-East, South-North, and vertical
#'   elevation axes respectively.
#'   In data with geographic coordinates, Z values are assumed to be in
#'   metres. In data with projected coordinates, Z values are assumed to have
#'   the same units as the X and Y coordinates.

Note: this change also benefits the slope_matrix() functions. See https://github.com/ITSLeeds/slopes/commit/7637f45fe1d9f2d18d3cfdd7362f000ce3c5edb6 for details.

[x] (now elevation_add()) - this is now described in README and in Get Started vignette. d9c40cac95ca8e106ed578daf85edc1a4f450fe4

* `slope_3d()` - it is not clear from the documentation that if you omit the raster object (`e = NULL`) then the function will download the necessary raster from MapBox - this is amazing functionality and should be advertised!

[x] ?slope_vector

* The documentation links in `?slope_vector` are not necessary since they link to the same help page.

Agreed, this has been fixed in https://github.com/ITSLeeds/slopes/commit/bcd382adaa8bd230b47a6ca8bce9286f937e961d

[x] slope_matrix_mean()

* I anticipated that there would be a `slope_matrix_mean()` that calculates the unweighted mean slope from a matrix, in addition to `slope_matrix()` and `slope_matrix_weighted()` - analogous to `slope_distance_mean()`.

This function has been added: https://itsleeds.github.io/slopes/reference/slope_matrix.html

[x] slope_xyz

* `slope_xyz` - Alternative functions that can be passed to `fun` should be documented.

Good point. This, plus other improvement to the docs for this function, have been made in https://github.com/ITSLeeds/slopes/commit/cb0e2e56127f6dedd83744e8b515581b373d4abd - wondering if the function should ever have been exported but it's there now and think it could be useful for others. Especially now it's better documented!

Examples

General Comments:
* All functions have examples that show the basic usage.

[x] How functions are called

* I think it would be good to standardize how functions are called in examples (sometimes they are called with `package::function()` and sometimes just `function()`). I think for `slopes` functions you probably don't need to use the `::` notation, as it assumed the package is loaded, and for functions from other packages you should standardize on either using (for example) `library(sf)` or `sf::function()` (I would lean towards the former to avoid the method dispatch issues I flagged in several examples below).

We call sf and tend to avoid the :: in the examples, although believe it is useful in some places in the package, e.g. to concisely to demonstrate that a function is from a particular package. See https://github.com/ITSLeeds/slopes/commit/e854cb4c7d6e0d701e7170d4b05749bef3a137b4 for details.

[x] Explanation of examples

* In general, I think the examples could contain a bit more explanation of what is happening and/or use more meaningful values in the examples (e.g., real lon/lat pairs and distances rather than simple integers) - this would facilitate understanding the intention and outputs.

We think that the use of simple interger values in the examples supports understanding by allowing people to see how they work from first principles, without needing to think of long lon/lat values. The examples in the vignettes, and the examples in the functions that do the 'heavy lifting' that are most commonly used by users, have real world examples. We would be happy to improve any specific examples that are not clearly explained. Overall, we have improved the documentation but we would welcome any further guidance on examples that should be improved.

[x] Examples that do not run

* Rather than commenting out examples that should not be automtically run, it would be better to wrap them in `\dontrun{}` or `\donttest{}`.

This is a good point that would also cause issues for the CRAN submission. Addressed in https://github.com/ITSLeeds/slopes/commit/fe5f12da7250f99b08921bd7cf90928a497242c2

[x] route_cyclestreets docs

Specific Issues:

* In the example for `cyclestreets_route`, I believe this contains the (commented out) code for generating the data, not an example of its use. I think this could be confusing for the user, and should probably belong in a script in the `data-raw` folder - the code also uses the `stplanr` package, which is not listed as a dependency in `slopes`. In order to run the commented-out `plot()` call, the `sf` package must be loaded to use the correct `plot()` method.

This has been fixed, as can be seen here: https://itsleeds.github.io/slopes/reference/cyclestreets_route.html

[x] dem_lisbon_raster example

* In the `dem_lisbon_raster()` example the `sf` package must be loaded to use the correct `plot()` method.

Fixed in https://github.com/ITSLeeds/slopes/commit/f21d4a99c6b80791a11ab750612bc9c61c732002

[x] slope_raster() example

* In the example for `slope_raster()`, the `sf` package must be loaded for the correct `[` method to be used (otherwise `r = lisbon_road_segments[1:3, ]` results in an invalid `sf` object).

This has been fixed in https://github.com/ITSLeeds/slopes/commit/4c7e2077d8fe271ed08804e66b6daecb606311ab

[x] CONTRIBUTING and CODE OF CONDUCT added,

Community guidelines

* DESCRIPTION file has URLs for GitHub and pkgdown pages, and a BugReports field +1

* There is currently no CONTRIBUTING file or instructions on how to contribute in the README.

* Currently there is no Code of Conduct, though it appears that will be [added upon acceptance to rOpenSci](https://devguide.ropensci.org/collaboration.html?q=conduct#coc-file).

[x] In progress

Functionality

The package ticks almost all of the boxes in the functionality checklist - Installation was quick and easy (I'm on a Mac), functional claims are satisfied, and performance was as expected. It has automated tests (see a few comments below), but there are a few parts of the rOpenSci packaging guidelines which are not met.

[x] In progress

Automated Tests

* Good test coverage (75%) though more tests that check edge cases and anticpated common user mistakes would be beneficial.

We have added new tests for the directed edge cases and the 'stop` functions. We think improved function behaviour should catch more user mistakes but welcome any further feedback on improving the tests further.

[x] Both added to imports

* Unconditional use of Suggested packages `geodist` and `colorspace` in tests could cause problems if they are not automatically installed.

[x] rOpenSci guidelines

rOpenSci Packaging Guidelines

* The package name is perfect.

* There is no CodeMeta file.

* It runs great on my Mac, and I see GitHub Actions is set up to check on all major platforms. R CMD check passed successfully (and quickly) locally on my machine.

* Please see other comments on API, function and argument naming, documentation, examples, testing, etc.

* Dependencies: There are quite a few packages in `Suggests:`. I definitely agree with reducing the number hard dependencies, I do wonder if some of these are important enough to be moved into `Imports` - I am specifically thinking about `geodist` and `pbapply`.

We agree with the comments. The package now imports geodist, colorspace and pbapply. There is now description of installing the packages and API token needed for downloading DEMs in the README after https://github.com/ITSLeeds/slopes/commit/327d5f4d83949b8e2eb6adaaadd5d80ee4a891ce. We have added a codemeta file. Function and argument names, and docs, have been improved thanks to the review process.

* The pkgdown site is great.
General Code Comments

The code generally follows good practices - it's legible and well structured, with little duplication (other than exceptions mentioned below). I think most functions would benefit from more thorough argument checking so that a user gets an early and informative error message if they try to use inappropriate argument values.

[x] Commented out code chunks

There are commented-out blocks of code sprinkled throughout, where the intent of them is not clear. Perhaps clarify with comments/TODOs and/or remove where appropriate.

We have removed the commented code, experiments from early versions of the package code. The codebase should be easier to read now.

Specific Code Comments

* Very nice use of diverging colour palettes in plots for positive/negative slopes.

[x] Caching

* `elevations_get()` - It looks like you are planning to implement caching functionality, which would be great. I think at the very least it would be worth enabling saving to disk via the `file` argument (currently marked as not implemented).

We have not yet added caching. We think think could represent 'mission creep'. We have, however, added a link to a description of caching tiles in the ceramic package in https://github.com/ITSLeeds/slopes/commit/3d324fea1d2daa6204be8cb66b3bbe0256897dc3

We have removed the unused file argument, simplifying the function: https://github.com/ITSLeeds/slopes/commit/66de731cf3bfcc4c516622b2d2ddacf2fcfcc4da

[x] Unhelpful CRS related error messages

* `plot_slope()` and `slope_raster()`: `lonlat` cannot be determined with `sf::st_is_longlat(r)` if `r` has no CRS as it returns `NA`, resulting in an error deeper in the stack. This should be caught early and an informative error should be thrown.

This is a good point. Fix documented in https://github.com/ITSLeeds/slopes/issues/36

[x] pbapply imports

* `pbapply` is used in `slope_matrices()`, a major workhorse function - I think it should be in `Imports` (and then no need to use `requireNamespace`).

Agreed and fixed. We were being overly averse to importing packages, to the point where it was increasing code complexity. Good news: switching this to Imports saved 6 lines of condition checking code: https://github.com/ITSLeeds/slopes/pull/31/commits/eb434d558cc7bc51bdf088466e668062efb51d6e

[x] elevation_extract() suggestion

* `elevation_extract()` works (perhaps unintentionally) directly with `sf` objects, however it returns different results than using it on the component coordinates, as illustrated in the examples. When done with an `sf` object it appears to return a list containing a vector of elevations for each line segment - which I think could be really useful! If you want this functionality I would recommend creating an S3 generic + methods and documenting the differences.
  ```
  m = sf::st_coordinates(lisbon_road_segments[1, ])
  e = dem_lisbon_raster
  elevation_extract(m, e)
  #>  [1] 3.267991 3.266818 3.265715 3.264418 3.263222 3.262339 3.261004 3.262972
  #>  [9] 3.264940 3.266908 3.268876 3.308304 3.461349 3.467100 3.439341 3.421206
  #> [17] 3.403916 3.387750 3.371584 3.356889 3.342193 3.327498 3.312644 3.297117
  #> [25] 3.282411 3.267716 3.253020 3.218922 3.210919 3.202916 3.194914 3.176672
  #> [33] 3.157027 3.191458 3.316882 3.410792 3.469193 3.487912 3.563260 3.640330
  #> [41] 3.717938
  elevation_extract(lisbon_road_segments[1, ], e)
  #> Warning in showSRID(uprojargs, format = "PROJ", multiline = "NO", prefer_proj
  #> = prefer_proj): Discarded datum Unknown based on GRS80 ellipsoid in Proj4
  #> definition
  #> Warning in showSRID(SRS_string, format = "PROJ", multiline = "NO", prefer_proj
  #> = prefer_proj): Discarded datum European Terrestrial Reference System 1989 in
  #> Proj4 definition
  #> [[1]]
  #>  [1] 3.370 3.382 3.394 3.406 3.419 3.431 3.104 3.116 3.128 3.140 3.152 3.233
  #> [13] 3.507 3.400 3.389 3.374 3.363 3.352 3.341 3.330 3.319 3.308 3.296 3.285
  #> [25] 3.274 3.263 3.252 3.241 3.230 3.219 3.208 3.315 3.451 3.560 3.433 3.543
  #> [37] 3.652 3.762
  ```

See https://github.com/ITSLeeds/slopes/commit/b153e6a27a2d9aa4cc375daf4531df05eb2ffdae thanks for the suggestion

[x] Done. we added na.rm = TRUE for z_min() and z_max().

* `slope_z_*()`: In `slope_z_mean()`, `mean()` is called with `na.rm = TRUE`, however `min()` and `max()` are not in their respective `slope_x_` functions - I recommend standardizing that, and/or exposing that argument to the `slope_z_*()` functions.

[x] sf vs sfc/sfg objects

* Many functions that say they must be used with `sf` objects could also be used with `sfc` and even `sfg` objects. It might be a good idea to support this, possibly by making a generic with methods for each? Or in many cases it may be fine to just rely on the S3 methods from the `sf` functions (e.g., `sf::st_coordinates()` has methods for `sf`, `sfc`, `sfg` and should "just work"). If this isn't a desirable direction, there should be more comprehensive checking of inputs to ensure good error messages if `r` is not `sf`.

For some functions such as slope_xyz() and slope_raster() passing geometry objects to routes seems to work already, we have updated the docs accordingly.

We found a case where passing an sfc object can cause issues, as illustrated in the reprex below from a previous version:

remotes::install_github("itsleeds/slopes")
#> Using github PAT from envvar GITHUB_PAT
#> Skipping install of 'slopes' from a github remote, the SHA1 (6ba560d8) has not changed since last install.
#>   Use `force = TRUE` to force installation
library(slopes)
elevation_add(lisbon_road_segment$geom)
#> Loading required namespace: ceramic
#> Preparing to download: 9 tiles at zoom = 18 from 
#> https://api.mapbox.com/v4/mapbox.terrain-rgb/
#> Warning in showSRID(uprojargs, format = "PROJ", multiline = "NO", prefer_proj
#> = prefer_proj): Discarded datum Unknown based on GRS80 ellipsoid in Proj4
#> definition
#> Error in UseMethod("st_geometry<-"): no applicable method for 'st_geometry<-' applied to an object of class "c('sfc_LINESTRING', 'sfc')"

^{Created on 2021-09-04 by the reprex package (v2.0.1)}

This wasted resources because it downloaded the tiles while the user could have saved time by knowing that it was going to fail due to the class early. We think that requiring sf inputs is reasonable given the package's aim of being a high level tool for adding slope variables to sf data frames that already have attributes.

We tried the following:

  if(is(routes, class2 = "sfc")) {
    routes = sf::st_geometry(rgeom3d_sfc)
  }

But it seemed to add complexity for little benefit. However, we have added more safety checks in a cases where sfc were found to fail. Happy to further iterate on this if there are other functions that don't work as expected with sfc objects but we expect and encourage most users to use sf objects as inputs, unless there are benefits of catering for and documenting sfc use cases that we have not considered https://github.com/ITSLeeds/slopes/commit/a3bfcec419c940a3a1369de9ebccfbb3f3c78e3f

[x] Code duplication in 'z' functions

* There is lots of code duplication in `z.R` - the internals of most of these could be abstracted out to one or two functions. In addition, using the `sf::st_z_range()` function might be useful here.

True that, hadn't noticed all that duplication, sloppy coding! Fixed in https://github.com/ITSLeeds/slopes/commit/fbe5740b936e1480dace8b10b3b1ffb7894cf700

[x] colorspaceadded to imports

* If the `Suggested` package `colorspace` is not installed, `plot_slope()` and `plot_dz()` will fail, due to the package being [called unconditionally in `make_pal()`](https://github.com/ITSLeeds/slopes/blob/9365709a95312b4feff3689a9e219c06af314167/R/plot_slope.R#L130).

[x] plot_slope() argument passing

* `plot_slope()` and `plot_dz()` - most of the aesthetic arguments in `plot_slope()` are not passed on to `plot_dz()` thus have no effect.

Good point. See fix in https://github.com/ITSLeeds/slopes/commit/9d7bdddc410c1f0fdd21fc40ef786c3ff35b9e12

[x] CRS transformations in elevation_get()

* `elevations_get()` - You transform the input `sf` object to lonlat to pass through to MapBox, but currently return the return the raster in its native CRS (Web Mercator I believe). This may not match the user's input, so they will likely have to transform one or the other for them to work together. This is probably fine, but the user should probably be given a message or a warning to advise them. Likewise, in `slope_3d()` it would be good to check that the CRSs of the two datasets match and throw an error otherwise.

Agreed, this was due to issues with raster reprojection code and new wkt CRS definitions. New approach with terra resolves this issue, as shown in https://github.com/ITSLeeds/slopes/commit/4476a381ff7150fa7d2ad8fcd5d3eb6f39dd47ae

[x] slope_3d (now slope_xyz()) code duplication

* In `slope_3d()` much of the code is duplicated and could be pulled out into small helper functions (in both conditions after checking `is.null(e)` on [line 261](https://github.com/ITSLeeds/slopes/blob/c018c5d65208728cea66a1a13451a4c568deef5f/R/slopes.R#L261) and in both conditions after [checking nrow()](https://github.com/ITSLeeds/slopes/blob/c018c5d65208728cea66a1a13451a4c568deef5f/R/slopes.R#L279)). Additionally the comment on [line 2810](https://github.com/ITSLeeds/slopes/blob/c018c5d65208728cea66a1a13451a4c568deef5f/R/slopes.R#L280) seems to be outdated since it does work with n > 1.

We agree that there was unnecessary duplicated code. We acted on this by removing redundant code, removing one of the conditions completely: https://github.com/ITSLeeds/slopes/commit/6ba560d8956c2ebe979a59223e4275a6a5190b4c

The function, now called elevation add, still has plenty of complexity and could probably be simplified further but, after attempting to do so were not sure how best to progress considering that additional helper functions also create complexity and lines of code, we could not find a way to add helper functions here that would lead to substantial improvements:

  if(is.null(dem)) {
    dem = elevation_get(routes)
    r_original = routes # create copy to deal with projection issues
    routes = sf::st_transform(routes, raster::crs(dem))
    suppressWarnings({sf::st_crs(routes) = sf::st_crs(r_original)})
    m = sf::st_coordinates(routes)
    mo = sf::st_coordinates(r_original)
    z = as.numeric(elevation_extract(m, dem, method = method, terra = terra))
    m_xyz = cbind(mo[, 1:2], z)
  } else {
    m = sf::st_coordinates(routes)
    z = as.numeric(elevation_extract(m, dem, method = method, terra = terra))
    m_xyz = cbind(m[, 1:2], z)
  }

[x] Function Argument Naming:

Function argument names e, r, and lonlat are used across multiple functions for different types, sometimes with different defaults and descriptions. I suggest standardizing these and using different names where appropriate. In general, I think preferring longer, informative argument names over single letters would be helpful. I created a table of the different ways these arguments are used and/or documented: Arg Name Function Arg Description Default value Notes r elevations_get Routes, the gradients of which are to be calculated. The object must be of class sf with LINESTRING geometries. None
r slope_3d Routes, the gradients of which are to be calculated. The object must be of class sf with LINESTRING geometries. None
r slope_raster Routes, the gradients of which are to be calculated. The object must be of class sf with LINESTRING geometries. None
r plot_slope A linestring with xyz dimensions None Different than others e slope_raster Raster overlapping with r and values representing elevations NULL
e elevation_extract Raster overlapping with r and values representing elevations None references r but no r in function signature e slope_3d Raster overlapping with r and values representing elevations NULL
e slope_vector, slope_distance, slope_distance_mean, slope_distance_weighted Elevations in same units as x (assumed to be metres) None
e slope_matrix, slope_matrix_weighted Elevations in same units as x (assumed to be metres) m[, 3] I think it would be helpful to describe that the default is taken from m (if m has three columns) but can be overridden. lonlat slope_xyz Are the coordinates in lon/lat order? TRUE by default NULL Default does not match description lonlat slope_raster Are the coordinates in lon/lat order? TRUE by default sf::st_is_longlat(r) Default does not match description lonlat plot_slope Are the coordinates in lon/lat order? TRUE by default sf::st_is_longlat(r) Default does not match description

Agreed, see comments above re argument names, we have now renamed r and e to routes, dem and elevations to reflect the different meanings of e and for clarity.

The first argument of plot_slope() is now route_xyz, reflecting the fact the function takes a single route with 'xyz' coordinates https://github.com/ITSLeeds/slopes/pull/35/commits/f9de5ccc70730c837325fcfd49ef84c705c5cbb1

Test with my own data:

I was playing and profiling a route of my own using OSM data and had a few thoughts:

* How does one know/set the direction a route is meant to be traveled (or for a river, the direction of flow)? OSM road segments (and I assume segments from many data sources) may not be divided up the way we want, or in sequential order. For example, the route I was dealing with had several road segments, but the `sf` object I retrieved using `osmdata` didn't have the road segments in order, which you can see in the first call to `plot_slope()`. I wonder if some functionality and/or a vignette showing how one might deal with this would be helpful? You can see my example workflow [here](https://gist.github.com/ateucher/14e32a352994eca71596c62df94d4841).

[x] Changepoints

* It would also be interesting/helpful to be able to break the route up at changepoints in slope according to the DEM - just an idea for future versions!

Agreed, this would be interesting. We have added a new vignette at https://itsleeds.github.io/slopes/articles/roadnetworkcycling.html that shows the importance of route breakpoints.

Robinlovelace commented 3 years ago

Image for new vignette

Robinlovelace commented 3 years ago

This is a great map but it adds ~13 MB to the package size as it's embedded in the html of the vignette. Plan: add link to interactive version.

temospena commented 3 years ago

Perfect, that is a much better idea!

Should I put it on branch rev2-fix?

On Sun, Jun 13, 2021 at 9:03 PM Robin @.***> wrote:

This is a great map but it adds ~13 MB to the package size as it's embedded in the html of the vignette. Plan: add link to interactive version.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ITSLeeds/slopes/issues/32#issuecomment-860263025, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJKLUXXXZLLCFKNHNYR4SSLTSUFJDANCNFSM46TUJCSA .

temospena commented 3 years ago

Just saw that you already did that on https://github.com/ITSLeeds/slopes/commit/1942e94f9f7c75948004ef9cf225e2e8911792ea :)

On Sun, Jun 13, 2021 at 9:44 PM rosa @.***> wrote:

Perfect, that is a much better idea!

Should I put it on branch rev2-fix?

On Sun, Jun 13, 2021 at 9:03 PM Robin @.***> wrote:

This is a great map but it adds ~13 MB to the package size as it's embedded in the html of the vignette. Plan: add link to interactive version.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ITSLeeds/slopes/issues/32#issuecomment-860263025, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJKLUXXXZLLCFKNHNYR4SSLTSUFJDANCNFSM46TUJCSA .

Robinlovelace commented 2 years ago

Draft reply to Andy's review: above. Draft reply to Dan's review, below...

Robinlovelace commented 2 years ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

* **Briefly describe any working relationship you have (had) with the
  package authors.**
  I shared an office with Robin Lovelace some years ago and we're still in touch.

* ☒ As the reviewer I confirm that there are no [conflicts of
  interest](https://devguide.ropensci.org/policies.html#coi) for me to
  review this work (If you are unsure whether you are in conflict,
  please speak to your editor _before_ starting your review).

Documentation

The package includes all the following forms of documentation:

* ☐ **A statement of need** clearly stating problems the software is
  designed to solve and its target audience in README

* ☒ **Installation instructions:** for the development version of
  package and any non-standard dependencies in README

* ☒ **Vignette(s)** demonstrating major functionality that runs
  successfully locally

* ☒ **Function Documentation:** for all exported functions

* [half an x] **Examples** (that run successfully locally) for all
  exported functions

* ☐ **Community guidelines** including contribution guidelines in the
  README or CONTRIBUTING, and DESCRIPTION with `URL`, `BugReports` and
  `Maintainer` (which may be autogenerated via `Authors@R`).

Functionality

* ☒ **Installation:** Installation succeeds as documented.

* ☒ **Functionality:** Any functional claims of the software been
  confirmed.

* ☒ **Performance:** Any performance claims of the software been
  confirmed.

* ☒ **Automated tests:** Unit tests cover essential functions of the
  package and a reasonable range of inputs and conditions. All tests
  pass on the local machine.

* ☐ **Packaging guidelines**: The package conforms to the rOpenSci
  packaging guidelines

Estimated hours spent reviewing: 6-8

* ☒ Should the author(s) deem it appropriate, I agree to be
  acknowledged as a package reviewer (“rev” role) in the package
  DESCRIPTION file.

Review Comments

Thanks for the invite to peer review. This is a generally awesome package that works well and is professionally assembled. I’m a reasonably competent R programmer but not an expert in package construction or testing, so apologies for any issues with my review.

Tested on a windows machine - runs well.

Comments on the above tickboxes first:

[x] Need statement

* Statement of need: does state the problem it solves but not the
  target audience (the ‘getting started’ article at
  https://itsleeds.github.io/slopes/articles/slopes.html states the
  need in a little more detail; the intro to that could go at the top
  of the readme?)

We have greatly improved the documentation and there is now a statement of need in the README, with a link for people wanting further details. See https://github.com/ITSLeeds/slopes/commit/7c2ac573129ef728ec17565d2307b5d198ca77b9

* Couldn’t find community guidelines though URL / bug reports are in
  DESCRIPTION

* Performance: benchmarks are slower for me but my machine’s quite old

* Automated tests are present though I don’t feel qualified to confirm
  they all pass

* Conforming to rOpenSci packaging guidelines: no CodeMeta file;
  function and argument naming is all great; package API excellent;
  code style is compact but great; readme is great, though it does use
  an example that requires a mapbox API auth key and this isn’t
  mentioned - more on that below.

Other comments

Only a couple of comments:

* The package README includes an example that automatically downloads
  raster tiles from Mapbox (a fantastically useful feature by the
  way).
  (`{r eval = F} lisbon_route_3d_auto = slope_3d(r = lisbon_route)`).
  However, it doesn’t say that you need a Mapbox API key for it to
  work. Might be worth adding that, or just a link? (Possibly with a
  `{r eval = F} usethis::edit_r_environ()` line to show how to add the
  key?) It’s such a super-useful part of the package, I think it’s
  worth making sure people know how to use it.

* The benchmarking test in the README downloads a dem_lisbon.tif file
  for one of the tests. That file wouldn’t work in either terra or
  raster for me - and anyway, if we’re comparing the two methods, I’m
  not sure why it can’t just use the already-available
  dem_lisbon_raster twice? (That’s how I ran the benchmark.)

We have updated the benchmark, which is now in a separate vignette, and which no longer requires data download, good point!

There are a couple of things I feel might be missing, possibly from the README intro or maybe something for the https://itsleeds.github.io/slopes/index.html page:

* Perhaps a little example of how to make your own route to use in the
  package. It’s quick enough that it wouldn’t bloat the README intro.
  Or could go in main documentation? E.g. using stplanr:

library(stplanr)
library(osrm)

#Trip between two Sheffield parks
#Coords nabbed from right-click in google maps
trip <- route(
  from = c(-1.493284835376672,53.39216628908168),
  to = c(-1.5066026758341629,53.368424563832754),
  route_fun = osrmRoute,
  returnclass = "sf"
)
#> Most common output is sf
mapview::mapview(trip)

#Is linestring so can be used in slopes
class(trip$geometry[1])

#Find slope of route... (using mapbox API)
sheffield_route_3d_auto = slopes::slope_3d(r = trip)
slopes::plot_slope(sheffield_route_3d_auto)

* It’s a slopes package, not an elevation package, but I wonder if it
  might be worth mentioning how to find elevation changes? Or provide
  a simple way to see elevation change summaries for particular
  slopes? I can imagine that being useful for e.g. assessing hilliness
  of bike journeys. Excuse my ignorance if this is already in the
  package - I couldn’t get the distance elevation profile example from
  ‘getting started’ to work for my Sheffield route above (though in
  theory I should be able to as sf::st_coordinates gives me the 3D
  matrix?) I could find some values like this:

library(tidyverse)
n = sf::st_coordinates(sheffield_route_3d_auto) %>% 
  data.frame %>% 
  mutate(elevationchange = Z-lag(Z))#change in elevation between line segments

#Total net elevation of trip
sum(n$elevationchange, na.rm = T)
#Total climb of trip
sum(n$elevationchange[n$elevationchange > 0], na.rm = T)

Would it be possible to provide a similar example that ties to the distance elevation profile example? Again, sorry - I did try using slope package functions to achieve this and failed.

We have updated the documentation, could you clarify it you can reproduce it now? Does the updated information in Get Started helps?

ropensci / slopes

Review for ropensci #32

Package Review

Documentation

Functionality

Review Comments

Review Checklist

Documentation

Statement of Need

Function Documentation

Examples

Community guidelines

Functionality

Automated Tests

rOpenSci Packaging Guidelines

General Code Comments

Specific Code Comments

Test with my own data:

Package Review

Documentation

Functionality

Review Comments

Comments on the above tickboxes first:

Other comments