pecan icon indicating copy to clipboard operation
pecan copied to clipboard

Bugfixes for `model2netcdf.ED2()`

Open Aariq opened this issue 1 year ago • 9 comments

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 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

  • [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?

Aariq avatar Jul 25 '22 14:07 Aariq

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.

Aariq avatar Jul 25 '22 21:07 Aariq

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.

Aariq avatar Jul 25 '22 21:07 Aariq

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.

infotroph avatar Jul 26 '22 04:07 infotroph

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.

Aariq avatar Jul 26 '22 14:07 Aariq

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.

Aariq avatar Jul 26 '22 20:07 Aariq

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.

Aariq avatar Jul 27 '22 21:07 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 avatar Aug 01 '22 13:08 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.

Aariq avatar Aug 01 '22 18:08 Aariq

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

dlebauer avatar Aug 09 '22 15:08 dlebauer