Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wbt_isobasins() crashed for any size >= 1e5, works well under #20

Open
Antoine-CSQN opened this issue Jun 15, 2020 · 10 comments
Open

wbt_isobasins() crashed for any size >= 1e5, works well under #20

Antoine-CSQN opened this issue Jun 15, 2020 · 10 comments

Comments

@Antoine-CSQN
Copy link

Antoine-CSQN commented Jun 15, 2020

Hello,
First of all thank you for this whitebox and its interface in R. I think it's a good tool for reproductible research in GIS, and it certainly makes some fastidious work with rgdal and raster easier.

I found what I think is a bug and I have no competence to fix it as I don't understand Rust.
When I try the to use the iso_basins() it works well for any size < 1e5 and crashes for any size > 1e5 (see exemple and console output below)

#Pre-processing
raw.dem <- dem.path #Any dem (mine is 10m*10m for a 1000 km² area)
wbt_feature_preserving_smoothing(raw.dem, "./smoothed.tif", filter=9, verbose_mode = TRUE)
wbt_breach_depressions("./smoothed.tif", "./breached.tif")
#Create isobasins for given size in cells
wbt_isobasins(dem = "./breached.tif", output = "./isobas_1km.tif", size = 1e4, wd = NULL, verbose_mode = TRUE) #works (exec time = 0.726s)
wbt_isobasins(dem = "./breached.tif", output = "./isobas_9pt99km.tif", size = 9.99e4, wd = NULL, verbose_mode = TRUE) #works (exec time = 0.688s)
wbt_isobasins(dem = "./breached.tif", output = "./isobas_10km.tif", size = 1e5, wd = NULL, verbose_mode = TRUE)  # Crash (immediately)

[1] "thread 'main' panicked at 'called Result::unwrap() on an Err value: ParseIntError { kind: InvalidDigit }', src\tools\hydro_analysis\isobasins.rs:170:21"
[2] "note: run with RUST_BACKTRACE=1 environment variable to display a backtrace"
attr(,"status")
[1] 101
Warning message:
In system(args2, intern = TRUE) :
running command 'C:/Users/me/Documents/R/R-4.0.0/library/whitebox/WBT/whitebox_tools.exe --run=isobasins --dem=./breached.tif --output=./isobas_10km.tif --size=1e+05 -v' had status 101

This bug was reproducted with two different Windows computer (10 standard and 10 Pro), an up-to-date whitebox install and two different R Version (3.61 and 4.0.0).

If you can have a look I would be grateful.

@jblindsay
Copy link
Collaborator

Hi Antoine,
I would suggest leaving the './' out of the file names. Please give that a try and let us know if it works.

@Antoine-CSQN
Copy link
Author

It throws the exact same error.
Problem is not with the syntax, it is with the value of the "size" argument.

@jblindsay
Copy link
Collaborator

jblindsay commented Jun 15, 2020

What happens when you use the non-scientific notation format (i.e. 1e5 = 100000) as the size parameter. I imagine that the R frontend passes parameters to the WBT backend as strings. Rust likely does not like parsing integer values in the scientific notation, but I'll need to check into that.

@giswqs
Copy link
Member

giswqs commented Jun 15, 2020

The error is likely caused by the scientific notation. Here are the R scripts behind the tool.

https://github.com/giswqs/whiteboxR/blob/master/R/hydro_analysis.R#L1047

wbt_isobasins <- function(dem, output, size, wd=NULL, verbose_mode=FALSE) {
  wbt_init()
  args <- ""
  args <- paste(args, paste0("--dem=", dem))
  args <- paste(args, paste0("--output=", output))
  args <- paste(args, paste0("--size=", size))
  if (!is.null(wd)) {
    args <- paste(args, paste0("--wd=", wd))
  }
  tool_name <- as.character(match.call()[[1]])
  wbt_run_tool(tool_name, args, verbose_mode)
}

https://github.com/giswqs/whiteboxR/blob/master/R/wbt.R#L273:1

wbt_run_tool <- function(tool_name, args, verbose_mode=FALSE) {
  wbt_init()
  wbt_exe <- wbt_exe_path()
  tool_name <- tool_name[!grepl("(whitebox|::)", tool_name)]
  tool_name <- substring(tool_name, 5)
  arg1 <- paste0("--run=", tool_name)
  args2 <- paste(wbt_exe, arg1, args, "-v")
  ret <- system(args2, intern = TRUE)
  if (verbose_mode == FALSE) {
    ret <- paste(tool_name, "-", utils::tail(ret, n = 1))
  }
  return(ret)
}

@Antoine-CSQN
Copy link
Author

Thanks for your attention but it still does not resolve the problem.
I tried :
wbt_isobasins(dem = "raster/whitebox/breached.tif", output = "raster/whitebox/isobas_10km.tif", size = 100000, wd = NULL, verbose_mode = TRUE)

And it throwed the same error

[1] "thread 'main' panicked at 'called Result::unwrap() on an Err value: ParseIntError { kind: InvalidDigit }', src\tools\hydro_analysis\isobasins.rs:170:21"
[2] "note: run with RUST_BACKTRACE=1 environment variable to display a backtrace"
attr(,"status")
[1] 101
Warning message:
In system(args2, intern = TRUE) :
running command 'C:/Users/acasquin/Documents/R/R-4.0.0/library/whitebox/WBT/whitebox_tools.exe --run=isobasins --dem=raster/whitebox/breached.tif --output=raster/whitebox/isobas_1km.tif --size=1e+05 -v' had status 101

It is really a threshold error as
wbt_isobasins(dem = "raster/whitebox/breached.tif", output = "raster/whitebox/isobas_9pt99km.tif", size = 99999, wd = NULL, verbose_mode = TRUE)
Works well

"Saving data..." "Output file written" "Elapsed Time (excluding I/O): 0.757s"

@brownag
Copy link
Member

brownag commented Aug 17, 2021

I can confirm that this is related to scientific notation, even if the user specifies size as a value like 100000 it seems that numbers that large automatically will be converted to scientific notation with default scipen options.

Using the latest version of the package I can successfully run the following:

wbt_run_tool("isobasins", "--dem=C:/Users/Andrew.G.Brown/Documents/R/win-library/4.0/whitebox/extdata/DEM.tif --output=foo.tif --size=1000000")

In contrast this fails:

> wbt_isobasins(dem = dem, output = "isobas_10km.tif", size = 100000, verbose_mode = TRUE)
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: ParseIntError { kind: InvalidDigit }', src\tools\hydro_analysis\isobasins.rs:187:57
stack backtrace:
   0:     0x7ff71c8fab5e - <unknown>
   1:     0x7ff71c919bfc - <unknown>
   2:     0x7ff71c8f6143 - <unknown>
   3:     0x7ff71c8fdccb - <unknown>
   4:     0x7ff71c8fd918 - <unknown>
   5:     0x7ff71c8fe49f - <unknown>
   6:     0x7ff71c8fe01f - <unknown>
   7:     0x7ff71c917270 - <unknown>
   8:     0x7ff71c9170a3 - <unknown>
   9:     0x7ff71c5e01af - <unknown>
  10:     0x7ff71beeaf24 - <unknown>
  11:     0x7ff71c79b3f9 - <unknown>
  12:     0x7ff71c798871 - <unknown>
  13:     0x7ff71c6b6b86 - <unknown>
  14:     0x7ff71c8fe796 - <unknown>
  15:     0x7ff71c79d337 - <unknown>
  16:     0x7ff71c92be55 - <unknown>
  17:     0x7ffec7ba7c24 - BaseThreadInitThunk
  18:     0x7ffec95ad721 - RtlUserThreadStart
Warning message:
In system(args2, intern = TRUE) :
  running command '"whitebox_tools.exe" --run=isobasins  --dem=C:/Users/Andrew.G.Brown/Documents/R/win-library/4.0/whitebox/extdata/DEM.tif --output=isobas_10km.tif --size=1e+05 -v' had status 101

I think we could probably handle "parameter_type":"Integer" parameters in the autogenerated tool codes by wrapping in as.integer() e.g. args <- paste(args, paste0("--size=", as.integer(size))) to avoid this.

> paste0("a=", as.integer(1e5))
[1] "a=100000"
> paste0("a=", 1e5)
[1] "a=1e+05"

Otherwise, folks can use a non-default scipen option to disable the scientific notation conversion

options(scipen = 100)
wbt_isobasins(dem = dem, output = "isobas_10km.tif", size = 100000)
#> isobasins - Elapsed Time (excluding I/O): 0.235s

@brownag
Copy link
Member

brownag commented Jan 18, 2022

Also, explicitly setting a numeric literal as an integer using L suffix e.g. 100000L works to resolve this for any Integer parameter parsing issues.

thread 'main' panicked at 'called Result::unwrap() on an Err value: ParseIntError { kind: InvalidDigit }', whitebox-tools-app/src/tools/hydro_analysis/isobasins.rs:190:57

> wbt_isobasins(dem = "DEM.tif", output = "isobas_10km.tif", size = 100000L)
isobasins - Elapsed Time (excluding I/O): 0.6s

@brownag
Copy link
Member

brownag commented Mar 30, 2023

I am going to close this issue via a PR hat implements some of the required steps to implement this.

The process that generates functions from the latest WhiteboxTools python script needs additional information parsed from the JSON specification of tool parameters. This is a step beyond the current heuristics which rely on the contents of the whitebox_tools.py file. The process to update internalwbttoolparameters dataset exists, it is just not currently integrated into the function code/.R file update process, but it is time that I do combine into a single operation, and make sure the "full" current update process for a new WhiteboxTools version is outlined.

@giswqs
Copy link
Member

giswqs commented Mar 30, 2023

You can look at the get_wbt_dict() function. I use it to extract tool parameters as a json file.
https://github.com/giswqs/whiteboxgui/blob/03dafef2f05f583507898d310ec90eeb73608568/whiteboxgui/whiteboxgui.py#L391

You can probably directly use this json file to build R functions
https://github.com/giswqs/whiteboxgui/blob/master/whiteboxgui/data/whitebox_tools.json

@brownag
Copy link
Member

brownag commented Mar 30, 2023

Yes, I used to build a custom .json file by running wbt_tool_parameters() on all tool names but opted to use the one included in whiteboxgui and your Python get_wbt_dict() function a while back for that. Much more consistent that way.

Currently that gets run like so for building the package datasets in data-raw folder:
https://github.com/giswqs/whiteboxR/blob/c98fe0318cd30db1dc4b3af40e05e6eea3612496/data-raw/wbttools2.R#L5-L10

Probably can move the reticulate based code to make the JSON file into PY2R/automate.py and then have the data-raw steps called on the updated json file (in data-raw folder) at end of that script, so only one script needs to be run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants