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

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

Closed
Aariq opened this issue Jul 22, 2022 · 2 comments · Fixed by #2985
Closed

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

Aariq opened this issue Jul 22, 2022 · 2 comments · Fixed by #2985
Assignees

Comments

@Aariq
Copy link
Collaborator

Aariq commented Jul 22, 2022

if (is.na(time)) {

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
Copy link
Member

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
Copy link
Collaborator Author

Aariq commented Jul 24, 2022

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.

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

Successfully merging a pull request may close this issue.

2 participants