Bugfixes for model2netcdf.ED2()

[Aariq]

Description

Went down a bit of a rabbit hole on this one...

Fixes several bugs related to model2netcdf.ED2(). The end goal is to fix #2974, but ended up having to deal with other issues along the way.

• Added tests for PEcAn.utils::mstmipvar() where there were none
• Refactors PEcAn.utils::mstmipvar()
• Changes default values in mstmipvar() to NULL instead of NA (why this is a good practice: https://stackoverflow.com/a/29620701). This is to fix PEcAn.utils::mstmipvar() condition has length > 1 error #2981
• Replaces test ED2 output for in tests/testthat/data so the example includes 2 PFTs
• Adds a pecan_checked.xml for testing the settings argument in test/testthat/data
• Fixes existing tests (closing ED tests are broken #1329) except for
• Fixes issue with model2netcdf.ED2() only reading one PFT (model2netcdf.ED2() only converts data for first PFT #2974) (and adds tests)
  • Looks like there was a time when functions looked for a character vector for pfts and the change to using the settings$pfts list was never completed.
• Set up the pftmapping dataset with usethis::use_data() and documented it. Surprised that it still gives a "global variables" type warning without explicitly assigning it 🤷‍♂️
• Also ran usethis::use_data_raw() to set up infrastructure to keep pftmapping.csv and convert to package dataset.

Motivation and Context

model2netcdf.ED2() was broken

Review Time Estimate

• Immediately
• Within one week
• When possible

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Unsure if new NULL defaults for mstmipvar args will cause problems anywhere else

Checklist:

• My change requires a change to the documentation.
• My name is in the list of CITATION.cff
• I have updated the CHANGELOG.md.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

Should I add a NEWS file to PEcAn.ED2?
Should I increment the version of PEcAn.ED2?

[Aariq]

@dlebauer, I'd like to look at the resulting .nc files with you and figure out if they're correct before merging this PR.

[Aariq]

.nc outputs don't have PFT as a dimension for variables that probably should have that dimension, but I think this is a separate issue for a different PR, so this PR is ready now.

[dlebauer]

Wow. Nice work!!! 👏🏻👏🏻👏🏻👏🏻
Thanks for taking the time to understand and improve how these functions work and to re-enable and add tests!

[dlebauer]

all test / check errors are 'Error: These files have changed since checkout. Check for out-of-sync autogenerated files, or for untracked files written inside the working tree'

TODO

• run ./scripts/generate_dependencies.R
• commit any resulting changes to docker/depends/pecan.depends.R

[infotroph]

@Aariq Sorry again for my long delay in reviewing -- As long as I typed them already, I'm posting the incomplete comments that I started before this was merged but then failed to post in time for them to be useful to you 😬

None of my comments are showstoppers, and in fact as I type this I realize my proposed dir.create additions in the tests aren't strictly needed because the unzip below them does the directory creation for us.

[infotroph]

tempfile gives a name but doesn't actually create a file there!

Suggested change

	outdir <- tempfile() #creates a directory
	outdir <- tempfile()
	dir.create(outdir, showWarnings = FALSE)

[dlebauer]

Looks like getting files and directories mixed up. Might be:

outfile <- tempfile()
file.create(outfile, showWarnings = FALSE)

[Aariq]

the intention here was to create a directory and I was instructed (by someone, forgot who) to use tempfile() to do this. I think a better way is probably to use a sub-directory in tempdir(). Might change it in the next PR.

[robkooper]

the good thing is that you are guaranteed this way to have a unique tempfolder, while if you create it you need to keep track how often this function has been called to make sure the folder you create is unique.

[dlebauer]

It is unique to the R session, but doesn't change between calls

[infotroph]

@Aariq I think I'm the one who said to use tempfile(), which does default to being inside the session temp directory... the gotcha is that what it returns is "here's a path that will be unique if you create a file-or-directory there" rather than a path it has created.

The other, maybe bigger gotcha is that tempdir() and tempfile() are such similarly named functions that do such completely different things. That's one of my top nominees for items that should be in the Inferno but aren't yet.

[infotroph]

Suggested change

	outdir <- tempfile()
	outdir <- tempfile()
	dir.create(outdir, showWarnings = FALSE)