dimfalk / kostra2010R

R interface for KOSTRA-DWD-2010R dataset
GNU General Public License v3.0
2 stars 0 forks source link

`calc_designstorm()`: catch invalid inputs #22

Closed dimfalk closed 2 years ago

dimfalk commented 2 years ago

e.g.

# input validation -----------------------------------------------------------

calc_modelrain(kostra, d = 85, tn = 100, type = "EulerII") calc_modelrain(kostra, d = 60, tn = 45, type = "EulerII") calc_modelrain(kostra, d = 120, tn = 100, type = "EulerIII")

dimfalk commented 2 years ago

https://github.com/falk-env/kostra2010R/blob/e3a0a31511bbee1126f87b3fd87068b887c610bf/R/calc_modelrain.R#L25-L47

Is this an acceptable way of handling user inputs?

dleutnant commented 2 years ago

I remember an online discussion about how to test for missing function arguments but can't find the link. Anyway, of course you could test with missing(), but as far as I remember, setting the default argument to NULL and testing with is.null() is the more preferred way.

If you test for a specific set of possibilities, I'd do sth like match.arg() and parse the result as Integer (at least in this case).

dimfalk commented 2 years ago

Thanks for the input. I'll give is.null() and match.arg() a try then.

Note:

Currently missing() can only be used in the immediate body of the function that defines the argument, not in the body of a nested function or a local call. This may change in the future.

dleutnant commented 2 years ago

another thought on this: the international literature usuallly uses "design storm" instead of "model rain".

dimfalk commented 2 years ago

That's a good one. DIN 4049 and Linguee were somehow not of great help here.. 😏

dimfalk commented 2 years ago

I had a look at match.arg() and I see a lot of use cases where I used if clauses before, like assigning units based on the parameter chosen, but I'm not sure if this is really suitable for input validation since this is not about flexible assigning of values but throwing informative error messages when incorrect values are provided.. Tell me if I'm wrong here.

But I came across stopifnot() looking quite promising:

set <- c(5, 10, 15, 20, 30, 45, 60, 90, 120, 180, 240, 360, 540, 720, 1080, 1440, 2880, 4320)

stopifnot("Duration has not been passed as an argument to the function." =
              !is.null(d),

            "Duration has to be of type numeric, e.g `d = 60`." =
              inherits(d, "numeric"),

            "Duration level specified is not allowed. Please pick a value from the following: \n " =
              (d %in% set))

Seems to work quite fine as intended but I'm not sure about how flexible message customization is implemented here.

Some more research regarding meaningful error messages, especially for checking arguments to functions, shows that {checkmate} seems to be a popular package for this use case.

Basically, upper code is reduced to:

set <- c(5, 10, 15, 20, 30, 45, 60, 90, 120, 180, 240, 360, 540, 720, 1080, 1440, 2880, 4320)

checkmate::assert_numeric(d)
checkmate::assertChoice(d, set)

Yeah, I know, another dependency - but what do you think, @dleutnant ?

dleutnant commented 2 years ago

I'm OK with {chekmate} (blog at r-hub).

dimfalk commented 2 years ago

Another great article, thanks! 😏