search

PecanProject / pecan

The Predictive Ecosystem Analyzer (PEcAn) is an integrated ecological bioinformatics toolbox.
www.pecanproject.org
Other
202 stars 234 forks source link

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

Closed Aariq closed 2 years ago

Aariq commented 2 years ago

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)

dlebauer commented 2 years ago

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.

Aariq commented 2 years ago

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.