slopes #420
Comments
|
Thanks for your submission @temospena. You Handling Editor is @annakrystalli. She will contact you soon with the first steps. |
|
Hello @temospena ! I'll be starting editors checks on the package shortly. Will ping you when I'm done with next steps. |
Editor checks:
Editor commentsMany 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).
|
|
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! |
|
Hi @annakrystalli As it is (62%), you may see that Thanks! |
|
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 |
|
Latest |
|
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
|
|
Good news on this
Fixed as documented in the latest links in ropensci/slopes#18 or at least it will be when the
Happy to proceed at this point 👍 took a look at that and it seemed hard + plots may change in near future
Thank you 🙏 ropensci/slopes#27
Cannot reproduce that, it passes on my set-up as documented below. 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: ropensci/osmdata#218 (comment) |
|
Hi @annakrystalli , Are there any further changes we should address for this revision process? |
|
Update: finally the test coverage is over 75% (just about!) on the automated coverage check 🎉
|
|
Excellent! Thanks @Robinlovelace! I'll now start looking for reviewers. |
|
@ropensci-review-bot seeking reviewers |
|
Please add this badge to the README of your package repository: [](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 |
terra::rast was not reading the local file (win10). changed to read directly from repo assets.
|
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!! 🎉 |
|
@ropensci-review-bot add @DanOlner to reviewers |
|
@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. |
|
@ropensci-review-bot add @ateucher to reviewers |
|
@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. |
|
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. |
|
Here it is, just for the record:
The README has be substantially updated and now documents the package's main functions: ropensci/slopes@dbacab8
More detailed responses in-line Response to review 2:
We have made substantial improvements to the vignettes. There is now an updated introductory Function Documentation
This has been done in ropensci/slopes@018d2d6. Many thanks for the tip.
This is a good point, we put the dataset docs together in a rush. They have been iteratively improved, including in this commit: 45068459072a8602985e22e5d7730d24122a1cdd
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 ropensci/slopes@7651276
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
The documentation has been improved as a result of this comment. See ropensci/slopes@18b0a5d for details. One question for the reviewers: should this function even be exported? It seems of little additional use compared with the original
The parameter name has been clarified and renamed:
Agreed, the
The description of the input #' @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
Agreed, this has been fixed in ropensci/slopes@bcd382a
This function has been added: https://itsleeds.github.io/slopes/reference/slope_matrix.html
Good point. This, plus other improvement to the docs for this function, have been made in ropensci/slopes@cb0e2e5 - 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!
We call
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.
This is a good point that would also cause issues for the CRAN submission. Addressed in ropensci/slopes@fe5f12d
This has been fixed, as can be seen here: https://itsleeds.github.io/slopes/reference/cyclestreets_route.html
Fixed in ropensci/slopes@f21d4a9
This has been fixed in ropensci/slopes@4c7e207
We have added new tests for the
We agree with the comments. The package now imports
We have removed the commented code, experiments from early versions of the package code. The codebase should be easier to read now.
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 We have removed the unused
This is a good point. Fix documented in ropensci/slopes#36
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: ropensci/slopes@eb434d5
See ropensci/slopes@b153e6a thanks for the suggestion
For some functions such as We found a case where passing an 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 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
True that, hadn't noticed all that duplication, sloppy coding! Fixed in ropensci/slopes@fbe5740
Good point. See fix in ropensci/slopes@9d7bddd
Agreed, this was due to issues with
We agree that there was unnecessary duplicated code. We acted on this by removing redundant code, removing one of the conditions completely: ropensci/slopes@6ba560d 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 The first argument of
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
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 ropensci/slopes@7c2ac57
We have updated the benchmark, which is now in a separate vignette, and which no longer requires data download, good point!
We have updated the documentation, could you clarify it you can reproduce it now? Does the updated information in Get Started helps? |
|
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 As to the question of whether to export One element of my review that I didn't see an answer to was Thanks again for the opportunity to review, and kudos for a great package! Andy |
|
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! 👏 |
|
@ropensci-review-bot approve |
|
Could not approve. Please, specify the name of the package. |
|
@ropensci-review-bot approve slopes |
|
Approved! Thanks @temospena for submitting and @DanOlner, @ateucher for your reviews! 😁 To-dos:
Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them 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. |
|
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... |
|
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... |
|
Heads-up, just put in a PR that changes the URLs in the docs: ropensci/slopes#42 Please take a look Anna and let us know if that looks good. |
|
Exciting news, it's on rOpenSci 🎉 https://github.com/ropensci/slopes |
|
Looks like we're in the team but I don't have push access at the moment... |
|
I've made @temospena admin of the repo again. You weren't listed as a collaborator @Robinlovelace (probably had rights through the previous organisation) but @temospena can now manage access to the repo. 👍 |
|
Redirecting to the robot...
|
|
To keep tracking:
|
|
Looking at the outstanding tasks...
Think that's done, not sure we ever had any styling tweaks.
Not done but not in any rush to do this as more important that https://github.com/itsleeds/slopes redirects at this stage I think, seems to be the top hit on Google.
Done
|
|
One comment that was not addressed by @ateucher was whether the package supports directed slopes. Answer, yes: it does now! See here for info (I've just fixed the docs but they are taking time to update on rOpenSci): https://docs.ropensci.org/slopes/articles/slopes.html#splitting-the-network-1 |
|
Lovely! Thanks for following up on that @Robinlovelace |
|
Hi all, this package looks great! Further to @Robinlovelace 's comment above, I have indeed been working on ways to auto-correct heights where interpolation of steep terrain results in lots of spurious elevation change. This rather seriously affected our cycling models of Chepstow where plenty of routes go along the top or bottom of cliffs; in that particular case I could correct them by hand but I'm pleased to say I can now offer a general solution. Temporary link to the draft paper here - please don't share more widely as it needs to go through peer review https://www.dropbox.com/s/b7id0n3g594po18/bayesian%20drape%20draft.pdf?dl=0 and repo here https://github.com/fiftysevendegreesofrad/BayesianDrape Comments are welcome - for one thing I should add a citation to the slopes R package, please let me know your preferred citation :) |
|
Wow, thanks for the heads-up Crispin! Likewise, let us know how we can cite your paper, looks like it solves the issue outlined in the nice infographic in the README really well. Given that the initial motivation of the slopes package was to provide gradient estimates for transport research, it strikes me as worthwhile to at least link to the method and maybe even to enable people to implement it. For a citation of the package, watch this space... Plan A is to submit it to JOSS, and here's a draft of the paper: https://github.com/ropensci/slopes/blob/master/paper.md |
|
Great, ok. I'll put a preprint of mine on arxiv in a few days just want to re read it first. Yours for the time being can be Lovelace, R. and Félix, R. Slopes R package. https://github.com/ropensci/slopes - and hopefully by the time mine is accepted I can update that to an "under review" for JOSS? Also as you suggested Robin - I'd be interested to compare the two methods, perhaps we could jointly write a paper on that? I'm going to feel sheepish if all my optimization could have been avoided by looking at maximum/minimum heights but I can see how it's going to work most of the time... |
|
For implementation you mentioned a way to call Python from R - that's presumably the easiest way forward. However BayesianDrape is admittedly a bit heavy on the dependency side, in particular it needs PyTorch which is about 1GB download (more if you actually want to run on GPU!) |
Date accepted: 2021-09-27
Due date for @DanOlner: 2021-06-09Submitting 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 @ateucher: 2021-06-09
Archive: TBD
Version accepted: TBD
Scope
Please indicate which category or categories from our package fit policies this package falls under: (Please check an appropriate box below. If you are unsure, we suggest you make a pre-submission inquiry.):
Explain how and why the package falls under these categories (briefly, 1-2 sentences):
The
slopespackage retreives elevation data via an interface to theceramicpackage, 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
sfpackage 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 #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
paper.mdmatching JOSS's requirements with a high-level description in the package root or ininst/.MEE Options
Code of conduct
The text was updated successfully, but these errors were encountered: