pecan
pecan copied to clipboard
Bugfixes for `model2netcdf.ED2()`
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 #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 #1329) except for
- Fixes issue with
model2netcdf.ED2()
only reading one 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 thesettings$pfts
list was never completed.
- Looks like there was a time when functions looked for a character vector for
- Set up the
pftmapping
dataset withusethis::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
- [x] Immediately
- [ ] Within one week
- [ ] When possible
Types of changes
- [x] 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.
- [x] My name is in the list of CITATION.cff
- [x] I have updated the CHANGELOG.md.
- [x] I have updated the documentation accordingly.
- [x] I have read the CONTRIBUTING document.
- [x] I have added tests to cover my changes.
- [x] All new and existing tests passed.
Should I add a NEWS file to PEcAn.ED2? Should I increment the version of PEcAn.ED2?
NOTE: this adds a 16mb file for testing (replacing a ~5mb one) I don't think I can get this file any smaller (this is the size even when ED2 simulation is run for a week) and it is needed for running tests.
Warnings from automated tests look like they're due to 3rd edition of testthat
. I'll make that explicit in DESCRIPTION and work on those warnings.
this adds a 16mb file for testing (replacing a ~5mb one) I don't think I can get this file any smaller
@Aariq (1) is this file already compressed, and if not could it be? (2) Can you say more about why the previous 5-MB file doesn't work for the updated test? CRAN has a pretty hard limit of 5 MB for the entire package, so if test cases really need to be this large then in the long run we'll likely need to find ways to store them outside the package.
this adds a 16mb file for testing (replacing a ~5mb one) I don't think I can get this file any smaller
@Aariq (1) is this file already compressed, and if not could it be? (2) Can you say more about why the previous 5-MB file doesn't work for the updated test? CRAN has a pretty hard limit of 5 MB for the entire package, so if test cases really need to be this large then in the long run we'll likely need to find ways to store them outside the package.
Compressed it's only 473 kb, so I'll give that a try. In fact, it will simplify things to have all the output files in a .zip, then uncompress them in a tempdir for testing, then remove.
The previous file was from an ED2 run with only a single PFT. The bug I was attempting to fix was specific to output from runs with multiple PFTs. It is maybe possible to testread_E_files()
and put_E_values()
with an "-E-" file with 2 PFTs (334 kb) but test read_T_files()
and put_T_values()
with a smaller "-T-" file (old one ~5mb) from a different run. The _T_
functions don't do anything with PFTs, I think, based on their arguments. I imagine it might cause problems (or if it doesn't, it probably should) with testing the overall model2netcdf.ED2()
function though if the output files were not all from the same run. I don't know enough about the contents of the .h5 files to know if they can be stripped down and still relevant for testing.
The failing checks are for undocumented datasets that I didn't touch. I have no idea what they are so I'm not planning on addressing these warnings in this PR.
Marking as a draft again because although bugs are fixed, I'm not sure model2netcdf.ED2 is working as intended. I can't figure out how to read in results separately by PFT. For example, in an output .nc file ncvar_get(x, "NPP")
returns a single long vector, even with multiple PFTs.
@dlebauer, I'd like to look at the resulting .nc files with you and figure out if they're correct before merging this PR.
.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.
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