Implement valley as initial method for corridor delineation #59
Conversation
|
|
||
| expect_equal(bucharest_boundary, bucharest_boundary_osm, tolerance = 1e-4) |
There was a problem hiding this comment.
It looks the boundary returned from the Overpass API have been updated, so this would not pass anymore (only runs locally, since we have skip_on_ci). I have modified the test so that we do not check anymore for equality with some previously-retrieved dataset, but we check for consistency with the bbox retrieved at the same time. I expect this check to be more robust..
| @@ -22,7 +14,8 @@ test_that("City boundary of Paris is returned without error", { | |||
| bb <- get_osm_bb(city_name) | |||
| crs <- get_utm_zone(bb) | |||
|
|
|||
| expect_no_error(get_osm_city_boundary(city_name, bb, crs)) | |||
There was a problem hiding this comment.
On my side this would be noted as an test without condition (not sure why, but probably checking that no error is raised is not a sufficient expect-clause?). I have thus added some stronger assert..
| }) | ||
|
|
||
| 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.
| expect_true(sf::st_equals_exact(valley, expected_valley, | ||
| par = 1.e-4, sparse = FALSE)) |
There was a problem hiding this comment.
When precision is set on the sf objects, this does not seem to be relevant anymore. I am also not 100% about what this par actually means (documentation is not so clear and I could not find clear examples of this). But the test seems to properly do the check, so probably no need to investigate further..
| @@ -124,31 +155,11 @@ dem_to_cog <- function(dem, fpath, output_directory = NULL) { | |||
| overwrite = TRUE) | |||
| } | |||
|
|
|||
| #' Reproject a raster or vector dataset to the specified | |||
There was a problem hiding this comment.
Reproject moved to utils, as it is more generic then its valley-specific usage.
| #' @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?
| #' @export | ||
| get_dem <- function(bb, resource = "STAC", ...) { |
There was a problem hiding this comment.
After trying to set this up "properly" with arbitrary input arguments, I have found the simplest and cleanest way was actually to define all input arguments (also considering they are not that many). 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.
No description provided.