Implement valley as initial method for corridor delineation #59
Conversation
| }) | ||
|
|
||
| test_that("raster data are correctly retrieved and merged", { | ||
| skip_on_ci() | ||
|
|
||
| dem <- load_raster(bb, asset_urls) |> CRiSp::reproject(32635) |
There was a problem hiding this comment.
In addition to loading the raster, this test would also require reprojection to work, and that the previously-retrieved DEM would be equal to the one loaded here. I have simplified this to make it more specific to the functionality of load_raster.
| #' @export | ||
| get_valley <- function(dem, river, crs) { |
There was a problem hiding this comment.
I think that data reprojection did not fit well in here, and it should be taken care outside of the function. I have only left a check on whether the datasets have the same CRS. I have, however, introduced the bbox to limit the area of interest, what do you think?
| #' - [AWS Copernicus DEM datasets](https://copernicus-dem-30m.s3.amazonaws.com/readme.html) | ||
| #' - [Data license](https://docs.sentinel-hub.com/api/latest/static/files/data/dem/resources/license/License-COPDEM-30.pdf) | ||
| # nolint end | ||
| default_stac_dem <- list( |
There was a problem hiding this comment.
Since these are some relevant default data parameters, and the two of them should always be paired together, I have thought about moving this to the global namespace.
|
Thanks for the review and for the plots @cforgaci! Concerning the example in the README, should I then update it with the options |
@fnattino, I would replace the example in the README with |
|
OK, so then I have updated the default initial method to be "valley" - so that the snippet in the README can remain simple and mostly based on defaults, but we now use the valley method. @cforgaci could I kindly ask you to update the graphics in the README file? |
yes, will doo, asap |
I updated the figure. |
Co-authored-by: Claudiu Forgaci <[email protected]>
…iSp into 52-corridor-valley-fn
|
Fixed the last issues, let me know whether you still see something that should still be addressed here @cforgaci - I will otherwise merge it |
Co-authored-by: Claudiu Forgaci <[email protected]>
Co-authored-by: Claudiu Forgaci <[email protected]>
No description provided.