pecan icon indicating copy to clipboard operation
pecan copied to clipboard

`PEcAn.utils::mstmipvar()` condition has length > 1 error

Open Aariq opened this issue 3 years ago • 2 comments

https://github.com/PecanProject/pecan/blob/fbfcd2f26e4bad20cff9009da49e04ad85c05314/base/utils/R/utils.R#L38

Should this line be is.null() instead of is.na()? When this gets called by model2netcdf.ED2(), time is a complex nested list.
This results in a warning in older versions of R but are errors in R > 4.2.0 (see NEWS) Tests run on older versions of R should be run with with _R_CHECK_LENGTH_1_CONDITION_=true to catch these as errors (I will open a separate issue for this though once I confirm if they currently are not run like this)

Aariq avatar Jul 22 '22 19:07 Aariq

If time is not given it will be NA by default. In that case we could replace with if (time == NA). If time is given, we can have other checks for validity (like, is it numeric? Alea’s Increasing so all(diff(time)) >0?, but that is beyond the scope of this bug.

dlebauer avatar Jul 22 '22 20:07 dlebauer

Oh, I see now. Typically NULL is used as a default, partially for this reason I think. If time is a list then if (time == NA) will cause the same error (also NA == NA returns NA, not TRUE). I'll poke around and see if changing default values to NULL and conditionals to if(!is.null()) is possible without breaking too much other stuff. If that seems too "risky" then wrapping in all() should fix it.

Aariq avatar Jul 24 '22 20:07 Aariq