search

ropensci / software-review

rOpenSci Software Peer Review.
286 stars 104 forks source link

slopes #420

Closed temospena closed 2 years ago

temospena commented 3 years ago

Date accepted: 2021-09-27 Submitting Author: Rosa Flx (@temospena) and Robin Lovelace (@robinlovelace) Repository: https://github.com/ITSLeeds/slopes
Version submitted: 0.0.1
Editor: @annakrystalli Reviewers: @DanOlner, @ateucher

Due date for @DanOlner: 2021-06-09 Due date for @ateucher: 2021-06-09

Archive: TBD
Version accepted: TBD 

Package: slopes
Title: Calculate Slopes of Roads, Rivers and Trajectories
Version: 0.0.0.9000
Authors@R: c(
    person("Robin", "Lovelace", email = "rob00x@gmail.com", role = c("aut","cre"),
      comment = c(ORCID = "0000-0001-5679-6536")),
    person("Joey", "Talbot", email = "j.d.talbot@leeds.ac.uk", role = c("aut"), 
      comment = c(ORCID = "0000-0002-6520-4560")),
    person("Rosa", "Félix", email = "rosamfelix@tecnico.ulisboa.pt", role = c("aut"), 
      comment = c(ORCID = "0000-0002-5642-6006"))
    )
Description: Functions and example data to support research into
  'longitudinal gradient' (slope) of linear geographic entities, as defined in the context of rivers
  by Cohen et al. (2018) <doi:10.1016/j.jhydrol.2018.06.066>.
  The package was initially developed to research road slopes but has also been
  used to calculate and visualise slopes of rivers and, using an open dataset
  published by Javier Ariza-López et al. (2019) <doi:10.1038/s41597-019-0147-x>, trajectories representing
  movement on roads.
  The package takes two main types of input data for slope calculation: vector geographic
  objects representing linear features, and raster geographic objects with elevation values
  (which can be downloaded using functionality in the package)
  representing a continuous terrain surface.
License: GPL-3
URL: https://github.com/itsleeds/slopes/, https://itsleeds.github.io/slopes/
Encoding: UTF-8
LazyData: true
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.1.1
Imports: 
    sf,
    raster,
    methods
Depends: 
    R (>= 2.10)
Suggests: 
    geodist,
    pbapply,
    terra,
    colorspace,
    knitr,
    rmarkdown,
    ceramic,
    bookdown
VignetteBuilder: knitr

Scope

The slopes package retreives elevation data via an interface to the ceramic package, enabling estimation of hilliness for routes anywhere worldwide even when local elevation data is lacking.

The main category is geospatial data: the package takes geographic lines objects and returns elevation data per vertex (providing the output as a 3D point geometry in the sf package by default) and per line feature (providing average gradient by default).

The target audience is academic researchers, practitioners, consultants and citizens interested in elevation profiles and slopes worldwide. A growing number of people working with geospatial data and require accurate estimates of gradient. An example of the demand for data provided by the package is a map showing gradients across Sao Paulo (Brazil, see image below) that has received more the 100 'likes' on Twitter and generated convervations: https://twitter.com/DanielGuth/status/1347270685161304069

Transport planning practitioners require accurate estimates of roadway gradient for estimating energy consumption, safety and mode shift potential in hilly cities (such as Lisbon, the case study city used in the README and examples in the documentation). Natural hazard researchers and risk assessors require estimates of linear gradient to inform safety and mitigation plans associated with project on hilly terrain. We believe that aquatic ecologists, flooding researchers and others could benefit from estimates of river gradient to support modelling of storm hydrographs, although we do not have experience in these domains.

The package fills a niche in R's geospatial package ecosystem. It is the only R package for easy slope computation of a line segment (or several) . It is also the only open source tool dedicated to the estimation of slopes on linear features in any language, as far as we are aware. We have benchmarked the results against a market leading proprietary implementation (ArcMap's 3D Analyst Extension). We would like to add more cross-comparisons of slope estimates obtained with this package and other implementations.

To add - see https://github.com/ropensci/software-review/issues/395 VERIFY:We believe the package complies with ethical guidelines in the Internet Research: Ethical Guidelines 3.0 document. It makes it easier for researchers to access and make use of data that is already in the public domain, under the conditions of the adhering to the conditions of the OdBL.

Technical checks

Confirm each of the following by checking the box.

This package:

(We have a test suite but with low coverage, we aim to increase coverage of functions tested.)

Publication options

JOSS Options - [x] The package has an **obvious research application** according to [JOSS's definition](https://joss.readthedocs.io/en/latest/submitting.html#submission-requirements). - [ ] The package contains a `paper.md` matching [JOSS's requirements](https://joss.readthedocs.io/en/latest/submitting.html#what-should-my-paper-contain) with a high-level description in the package root or in `inst/`. - [ ] The package is deposited in a long-term repository with the DOI: - (*Do not submit your package separately to JOSS*)
MEE Options - [ ] The package is novel and will be of interest to the broad readership of the journal. - [ ] The manuscript describing the package is no longer than 3000 words. - [ ] You intend to archive the code for the package in a long-term repository which meets the requirements of the journal (see [MEE's Policy on Publishing Code](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/journal-resources/policy-on-publishing-code.html)) - (*Scope: Do consider MEE's [Aims and Scope](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/aims-and-scope/read-full-aims-and-scope.html) for your manuscript. We make no guarantee that your manuscript will be within MEE scope.*) - (*Although not required, we strongly recommend having a full manuscript prepared when you submit here.*) - (*Please do not submit your package separately to Methods in Ecology and Evolution*)

Code of conduct

melvidoni commented 3 years ago

Thanks for your submission @temospena. You Handling Editor is @annakrystalli. She will contact you soon with the first steps.

annakrystalli commented 3 years ago

Hello @temospena ! I'll be starting editors checks on the package shortly. Will ping you when I'm done with next steps.

annakrystalli commented 3 years ago

Editor checks:

Editor comments

Many thanks again for the submission of this useful package @temospena & @Robinlovelace. I've now completed initial editors checks. Comments below, the most pressing one regarding test coverage (see below).

devtools::check(pkg_dir)

Checks throw up a single note. Seems minor but would need addressing seeing as you wish to submit to CRAN.

checking Rd cross-references ... NOTE
  Package unavailable to check Rd xrefs: ‘stplanr’

0 errors ✓ | 0 warnings ✓ | 1 note x

goodpractice::gp()

A few points to look into were returned by goodpractice::gp(). The most critical being the low test coverage. To accept a package we require a minimum of 75% coverage. Is there a reason only 10% of code lines are currently tested?

  ✖ write unit tests for all functions, and all
    package code in general. 10% of code lines are covered by
    test cases.

    R/plot_slope.R:30:NA
    R/plot_slope.R:31:NA
    R/plot_slope.R:32:NA
    R/plot_slope.R:33:NA
    R/plot_slope.R:34:NA
    ... and 130 more lines

  ✖ add a "BugReports" field to DESCRIPTION, and
    point it to a bug tracker. Many online code hosting
    services provide bug trackers for free,
    https://github.com, https://gitlab.com, etc.
  ✖ use '<-' for assignment instead of '='. '<-' is
    the standard, and R users and developers are used it and
    it is easier to read your code for them if you use '<-'.

    R/plot_slope.R:13:12
    R/plot_slope.R:30:5
    R/plot_slope.R:31:5
    R/plot_slope.R:32:5
    R/plot_slope.R:33:5
    ... and 96 more lines

  ✖ avoid long code lines, it is bad for
    readability. Also, many people prefer editor windows that
    are about 80 characters wide. Try make your lines shorter
    than 80 characters

    R/data.R:3:1
    R/data.R:5:1
    R/data.R:15:1
    R/data.R:18:1
    R/data.R:29:1
    ... and 24 more lines

  ✖ fix this R CMD check NOTE: Namespace in Imports
    field not imported from: ‘methods’ All declared Imports
    should be used.
  ✖ avoid 'T' and 'F', as they are just variables
    which are set to the logicals 'TRUE' and 'FALSE' by
    default, but are not reserved words and hence can be
    overwritten by the user.  Hence, one should always use
    'TRUE' and 'FALSE' for the logicals.

    R/z.R:NA:NA

More detailed breakdown of the low coverage revealled bycovr::package_coverage()

slopes Coverage: 10.60%
R/plot_slope.R: 0.00%
R/slope_get.R: 0.00%
R/z.R: 16.67%
R/slopes.R: 16.90%

i've also highlighted a few potential spelling issues returned by devtools::spell_check()

datast            README.md:87
                  README.Rmd:83
formular          slopes.Rmd:96
implemenation     slopes.Rmd:85
introdution       slopes.Rmd:2
lenght            slopes.Rmd:51
purpouse          slopes.Rmd:33
Robinlovelace commented 3 years ago

Thanks for the initial editor checks, some very useful info in there. Update on this, we're working on it - coverage is now at 40% and we plan to get it to over 75% and let you know. Busy start to the year has slowed this project down a bit, more soon!

temospena commented 3 years ago

Hi @annakrystalli The package has coverage of 75% when tests run on a computer that has a MAPBOX api key in the environment (see: https://github.com/ITSLeeds/slopes/issues/26 ) - we would like to add this to the tests but we are not sure how... any clues?

As it is (62%), you may see that slope_get is the one with zero coverage, when without an api key. image https://codecov.io/gh/itsleeds/slopes/tree/master/R

Thanks!

Robinlovelace commented 3 years ago

Just to chime in here also, I've taken a look at adding tests for the plots but, even after looking at the great tests on ggplot2 and having a try, am not sure how to make it work. We envisage future functionality will be tested so the tests should go up further. Any further comments welcome, tests are important for sure and I think we've got the key slope-calculating functions covered. Regarding other issues, we've fixed most of the other issues flagged and the gp() results are now better : )

Robinlovelace commented 3 years ago

Latest gp() results below on my computer after a few long linestring fixes. One question, any idea how to fix the one about 'methods' being used? methods::*() functions are used in the package so a bit :confused: about that one. Other than that, it's definitely an improvement and can get even better no doubt in terms of the tests on computers that don't have the MAPBOX API key environment variable set.

── GP slopes ──────────────────────────────────────────────────────────────

It is good practice to

  ✖ write unit tests for all functions, and all package code in
    general. 75% of code lines are covered by test cases.

    R/plot_slope.R:30:NA
    R/plot_slope.R:31:NA
    R/plot_slope.R:77:NA
    R/plot_slope.R:78:NA
    R/plot_slope.R:79:NA
    ... and 34 more lines

  ✖ use '<-' for assignment instead of '='. '<-' is the standard,
    and R users and developers are used it and it is easier to
    read your code for them if you use '<-'.

    R/plot_slope.R:13:12
    R/plot_slope.R:30:6
    R/plot_slope.R:59:9
    R/plot_slope.R:79:7
    R/plot_slope.R:80:9
    ... and 136 more lines

  ✖ fix this R CMD check NOTE: Namespace in Imports field not
    imported from: ‘methods’ All declared Imports should be used.
  ✖ fix this R CMD check WARNING: LaTeX errors when creating PDF
    version. This typically indicates Rd problems.
annakrystalli commented 3 years ago

Hello @temospena & @Robinlovelace, thanks for all the great work done so far on the tests! I'll try and answer your questions:

Test failure on check()

I'm getting the following failed test now:

── Failure (test-plots.R:24:3): plotting functions work ────────────────────────
make_pal(p, b[1:4]) (`actual`) not equal to c("#FF0000", "#00FF00", "#0000FF") (`expected`).

`actual`:   "#FF0000FF" "#00FF00FF" "#0000FFFF"
`expected`: "#FF0000"   "#00FF00"   "#0000FF"
Robinlovelace commented 3 years ago

Good news on this 

* **mapbox API key**. I think the simplest way would be add the API key to your repo's [.secrets](https://docs.github.com/en/actions/reference/encrypted-secrets)?

Fixed as documented in the latest links in https://github.com/ITSLeeds/slopes/issues/18 or at least it will be when the sf package works on Mac, which should be in the next day or so when the binaries update:

image

* **testing plots** I think it's fine at this point to proceed without tests for the time being and have that as a potential future enhancement. Perhaps a simple tests you could include would be to test against a reference snapshot of a saved plot with [`expect_snapshot_file()`](https://testthat.r-lib.org/reference/expect_snapshot_file.html)

Happy to proceed at this point :+1: took a look at that and it seemed hard + plots may change in near future

* `methods` dependency: It looks like `methods` is a [special case](https://github.com/hadley/r-pkgs/issues/203). Having a look at this [more detailed documentation on namespacing](https://r-pkgs.org/namespace.html) and in particular the [S4 section](https://r-pkgs.org/namespace.html#import-s4) it looks like the workaround is to import the `methods` functions you need in the function roxygen2 documentation.  I've opened a PR that seems to fix this issue.

Thank you :pray: https://github.com/ITSLeeds/slopes/pull/27

* On that subject of dependencies, why is `ceramic` not specified as an import? It's being used within a function with a message to install it manually if missing? If there is some sort of issue you are trying to get round, it's best to outline any necessary additional installation instructions in the installation part of the `README`.

ceramic does not have core functionality and is a bit fiddly to set-up. Wanting to minimise dependencies for fast user install times. Could promote from Suggests to Imports if the +s of doing so outweigh the fairly minor -s of extra install time and dependencies for some users.

I'm getting the following failed test...

Cannot reproduce that, it passes on my set-up as documented below.

``` > testthat::test_file("tests/testthat/test-plots.R") ══ Testing test-plots.R ══════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════ [ FAIL 0 | WARN 0 | SKIP 0 | PASS 7 ] Done! > sessionInfo() R version 4.0.4 (2021-02-15) Platform: x86_64-pc-linux-gnu (64-bit) Running under: Ubuntu 20.04.2 LTS Matrix products: default BLAS: /usr/lib/x86_64-linux-gnu/blas/libblas.so.3.9.0 LAPACK: /usr/lib/x86_64-linux-gnu/lapack/liblapack.so.3.9.0 locale: [1] LC_CTYPE=en_GB.UTF-8 LC_NUMERIC=C LC_TIME=en_GB.UTF-8 LC_COLLATE=en_GB.UTF-8 LC_MONETARY=en_GB.UTF-8 [6] LC_MESSAGES=en_GB.UTF-8 LC_PAPER=en_GB.UTF-8 LC_NAME=C LC_ADDRESS=C LC_TELEPHONE=C [11] LC_MEASUREMENT=en_GB.UTF-8 LC_IDENTIFICATION=C attached base packages: [1] stats graphics grDevices utils datasets methods base other attached packages: [1] testthat_3.0.2 slopes_0.0.0.9000 loaded via a namespace (and not attached): [1] Rcpp_1.0.6 lattice_0.20-41 prettyunits_1.1.1 class_7.3-18 ps_1.6.0 assertthat_0.2.1 rprojroot_2.0.2 [8] digest_0.6.27 utf8_1.2.1 R6_2.5.0 evaluate_0.14 e1071_1.7-6 pillar_1.5.1 rlang_0.4.10 [15] rstudioapi_0.13 callr_3.5.1 raster_3.4-5 rmarkdown_2.7 desc_1.3.0 devtools_2.3.2 stringr_1.4.0 [22] proxy_0.4-25 compiler_4.0.4 xfun_0.22 pkgconfig_2.0.3 pkgbuild_1.2.0 htmltools_0.5.1.1 tidyselect_1.1.0 [29] tibble_3.1.0 roxygen2_7.1.1 codetools_0.2-18 fansi_0.4.2 crayon_1.4.1 dplyr_1.0.5 withr_2.4.1 [36] sf_0.9-8 waldo_0.2.5 commonmark_1.7 grid_4.0.4 lifecycle_1.0.0 DBI_1.1.1 magrittr_2.0.1 [43] units_0.7-1 KernSmooth_2.23-18 cli_2.3.1 stringi_1.5.3 cachem_1.0.4 diffobj_0.3.3 fs_1.5.0 [50] remotes_2.2.0 sp_1.4-5 xml2_1.3.2 ellipsis_0.3.1 generics_0.1.0 vctrs_0.3.6.9000 rematch2_2.1.2 [57] tools_4.0.4 rcmdcheck_1.3.3 glue_1.4.2 purrr_0.3.4 yaml_2.2.1 processx_3.4.5 pkgload_1.2.0 [64] fastmap_1.1.0 colorspace_2.0-0 xopen_1.0.0 sessioninfo_1.1.1 classInt_0.4-3 memoise_2.0.0 knitr_1.31 [71] usethis_2.0.1.9000 ```

A number of tests are producing warnings. I think this is the same as the issue documented by @mpadge (any ideas on a solution, suppress the warnings in the affected functions?) here: https://github.com/ropensci/osmdata/issues/218#issuecomment-801921318

temospena commented 3 years ago

Hi @annakrystalli ,

Are there any further changes we should address for this revision process? Thanks!

Robinlovelace commented 3 years ago

Update: finally the test coverage is over 75% (just about!) on the automated coverage check :tada:

image

annakrystalli commented 3 years ago

Excellent! Thanks @Robinlovelace! I'll now start looking for reviewers.

annakrystalli commented 3 years ago

@ropensci-review-bot seeking reviewers

ropensci-review-bot commented 3 years ago

Please add this badge to the README of your package repository:

[![Status at rOpenSci Software Peer Review](https://badges.ropensci.org/420_status.svg)](https://github.com/ropensci/software-review/issues/420)

Furthermore, if your package does not have a NEWS.md file yet, please create one to capture the changes made during the review process. See https://devguide.ropensci.org/releasing.html#news

annakrystalli commented 3 years ago

Apologies for the massive delay in moving to the next stage. It's taken me ages to find the second reviewer but I finally have!! 🎉

annakrystalli commented 3 years ago

@ropensci-review-bot add @DanOlner to reviewers

ropensci-review-bot commented 3 years ago

@DanOlner added to the reviewers list. Review due date is 2021-06-09. Thanks @DanOlner for accepting to review! Please refer to our reviewer guide.

annakrystalli commented 3 years ago

@ropensci-review-bot add @ateucher to reviewers

ropensci-review-bot commented 3 years ago

@ateucher added to the reviewers list. Review due date is 2021-06-09. Thanks @ateucher for accepting to review! Please refer to our reviewer guide.

DanOlner commented 3 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

Documentation

The package includes all the following forms of documentation:

Functionality

Estimated hours spent reviewing: 6-8

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:

Other comments

Only a couple of comments:

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:

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)
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.

annakrystalli commented 3 years ago

Thank you for your timely review @DanOlner!

@temospena, I suggest waiting for @ateucher 's review before responding.

@ateucher, just let me know if you are stuck with anything or need more time. 👍

ateucher commented 3 years ago

I'm good thanks @annakrystalli! My review will be in later today

ateucher commented 3 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

Documentation

The package includes all the following forms of documentation:

Functionality

Estimated hours spent reviewing: 8-10

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()).

Installation instructions

Vignette(s)

Function Documentation

General Comments:

Specific documentation issues:

Examples

General Comments:

Specific Issues:

Community guidelines

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.

Automated Tests

rOpenSci Packaging Guidelines

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.

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.

Specific Code Comments

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

Test with my own data:

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

ateucher commented 3 years ago

Sorry for the slight delay! It's still June 9 where I live ;)

annakrystalli commented 3 years ago

No problem at all @ateucher! And many thanks for the thorough review.

@temospena, over to you to respond to the reviewers comments 😊

temospena commented 3 years ago

Thank you very much for your excellent comments! We have started the revision process and will let you know as soon as it is completed.

annakrystalli commented 3 years ago

Hello @temospena ! Just wanted to check in with you about how the revisions are going.

Feel free to ask for any clarification if you are unsure of anything at any point.

annakrystalli commented 2 years ago

Hello again!

I appreciate it's the middle of people's holidays. Would be great to get an update on the status of your changes even if it's just a note about when you might be ready to respond.

And of course feel free to reach out with any question or requests for clarification from myself or the reviewers. 👍

Robinlovelace commented 2 years ago

Thanks for the reminder, holidays and other deadlines have slowed down progress on this but progress there is. See here for a merged PR that tackles many of the issues reported by reviewer 2: https://github.com/ITSLeeds/slopes/pull/31

More soon-ish... there is lots of value in the comments so committed to acting on them :sparkles:

Robinlovelace commented 2 years ago

Update on this, we're making progress! We've addressed around half of the issues and think the others are doable. See here for updates (23 down, 23 to go at the time of writing): https://github.com/ITSLeeds/slopes/issues/32

annakrystalli commented 2 years ago

Good to hear! Thanks for the update!

Robinlovelace commented 2 years ago

Exciting update, 48 down, 3 to go! The last three will take some time but are definitely doable, almost there, thanks for patience. This has been harder but more rewarding than responding to academic peer review comments for @temospena and I but we're glad for all the feedback and we're almost there. If anyone has any suggestions/support on the final issues/discussion, see our open mega issue here for the unfinished points yet to be responded to: https://github.com/ITSLeeds/slopes/issues/32

ateucher commented 2 years ago

Wow @temospena and @Robinlovelace - I poked my head in after not looking for a while, and the work and improvements you're doing are amazing! Kudos, looking forward to seeing the end product!

annakrystalli commented 2 years ago

👏👏👏👏 great work and so glad you're finding the review process enjoyable. It's definitely a great learning experience! Look forward to seeing the finished product. Let us know when you're ready!

temospena commented 2 years ago

Hi! We just finished reviewing the package. Please see the responses to reviewers at: https://github.com/ITSLeeds/slopes/issues/32

It had major improvements and we consider that it is much better now.

We started by responding to comments from @ateucher, so many of the comments from @DanOlner were already covered by the previous reply. Any further comments or suggestions are very welcome.

We can write the responses here, if that would be useful.

Thank you for the great suggestions!

Robinlovelace commented 2 years ago

Adding to what Rosa says, thanks for an in depth review guys, we've made 100s of small changes in response and confident the package, and it's users, will benefit greatly from the process! There are definitely still things that could be changed for the better, we have talked about sub-sampling from vertices to calculate overall segment slope, reducing variability associated with linear segments that run perpendicular to steep slopes, something that @fiftysevendegreesofrad has explored and has an method to handle I think, and we could implement a feature to get only elevation points where they are needed instead of for the whole raster dataset surrounding a particular area, as per https://github.com/ITSLeeds/slopes/issues/21, for example. But overall my feeling is that the package solves the problem that it was set out to solve and I look forward to using it, and see it used in applied settings. We weren't sure what the convention is but if it's to post complete replies to the comments on this issue happy to do that also. Hope you find our changes move the package forward, any follow-ups v. welcome, and looking forward to seeing what new slope datasets generated by this package can do!

annakrystalli commented 2 years ago

Great news!! Congratulations on all the work you've put into this so far! 👏🚀

In terms of your question regarding, it is indeed convention to post replies to each of the authors comments. If some are duplicate, it's fine to respond to them in one go. We do need to make it as easy as possible for reviewers to understand what has been done in response to their comments and, indeed if some comments where not addressed why. I know it's more effort but it will make it much easier for reviewers to sign it off and it also forms a record of all actions in the review issue itself. Almost there!

annakrystalli commented 2 years ago

Ah sorry! I just noted that you've already got all the comments in the issue you linked to! So not much work I guess. Might as well just paste it here too just for the record.

annakrystalli commented 2 years ago

@ateucher & @DanOlner, over to you! Please have a look at the comprehensive response by the authors ad let us know if they've sufficiently responded to your comments or whether you feel there are any more changes required?

DanOlner commented 2 years ago

Apologies, I've got a heap of teaching prep to do, might not get to this for a couple of weeks (or possibly 3...) Really sorry to slow things down when you've put in so much work! If I get some time I'll look before then.

temospena commented 2 years ago

Here it is, just for the record:

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

More detailed responses in-line

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()).

* 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:

* 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.

* The included datasets are great - nice tidy real-world examples to play with.
* 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

* 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:

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

* 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.

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.

* `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!

* `plot_dz`:

  * All argument documentation should be expanded, at a minimum stating the defaults, the accepted options (and/or type/format of inputs).
  * 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.
  * 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.
  * 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
* `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.

* `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.

* `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!
* 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

* 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

* `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.

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.

* 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.

* 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

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

* 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

* 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

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).

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.

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.

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

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.

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.
* `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

* `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

* `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

* `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

* `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.
* 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

* 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

* 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).
* `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

* `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

* 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)
  }

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).
* 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.

Response to Reviewer 1

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:

* 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?

ateucher commented 2 years ago

My congratulations to @temospena and @Robinlovelace - that was a ton of work, and I think it's much better. I hope you found it worth it! In my opinion you've more than adequately addressed my comments and I'm happy to recommend acceptance. I especially love the new suite of vignettes, in particular roadnetworkcycling.Rmd. In the cases where you chose not to implement my suggestions, your reasons were well explained and I'm satisfied with that.

As to the question of whether to export elevation_extract() - I don't see any harm in exporting it even if it is a thin wrapper, especially now that you've documented nicely.

One element of my review that I didn't see an answer to was "How does one know/set the direction a route is meant to be traveled (or for a river, the direction of flow)?" - however I think now that this is out of scope for this package and is maybe answered elsewhere (perhaps stplanr)?

Thanks again for the opportunity to review, and kudos for a great package!

Andy

annakrystalli commented 2 years ago

Thank you @ateucher ! I also agree that I'm very happy with the responses to reviewers comments and am happy to approve the package in @DanOlner's place given he is really busy atm. @DanOlner would you be happy for me to do so?

annakrystalli commented 2 years ago

OK folks, I'm going to go ahead and approve this package! Really well done @temospena and @Robinlovelace on the massive work carried out during the review and many many thanks to both reviewers @DanOlner & @ateucher for their efforts! 👏

annakrystalli commented 2 years ago

@ropensci-review-bot approve

ropensci-review-bot commented 2 years ago

Could not approve. Please, specify the name of the package.

annakrystalli commented 2 years ago

@ropensci-review-bot approve slopes

ropensci-review-bot commented 2 years ago

Approved! Thanks @temospena for submitting and @DanOlner, @ateucher for your reviews! :grin:

To-dos:

Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them "rev"-type contributors in the Authors@R field (with their consent).

Welcome aboard! We'd love to host a post about your package - either a short introduction to it with an example for a technical audience or a longer post with some narrative about its development or something you learned, and an example of its use for a broader readership. If you are interested, consult the blog guide, and tag @stefaniebutland in your reply. She will get in touch about timing and can answer any questions.

We maintain an online book with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved.

Last but not least, you can volunteer as a reviewer via filling a short form.

DanOlner commented 2 years ago

Awesome news on approval. Sorry I didn't manage to get round to responding to the incredible amount of work put in after the reviews. Also, sorry Anna - I somehow didn't see your question for me seven days ago. p.s. I know someone working on green space metrics who may get good use out of this package...

Robinlovelace commented 2 years ago

This is fantastic news, many thanks @annakrystalli and all! One quick question: what is the process for submission to JOSS at this point? Discussing next steps with Rosa now...