Skip to content

integration test for LandR-CBM simplified#102

Open
suz-estella wants to merge 1 commit intodevelopmentfrom
suz-landR-test
Open

integration test for LandR-CBM simplified#102
suz-estella wants to merge 1 commit intodevelopmentfrom
suz-landR-test

Conversation

@suz-estella
Copy link
Contributor

Closes #101

I think it's good that we have reviewed this changed result and have decided we now have the correct species IDs making their way to the module.

For the update to the integration test: I think it might be best to take this moment to simplify it instead of setting ourselves to have to keep maintaining it over time. This PR removes specific validations to instead just check that the simulation runs without error. It also shortens the simulation time span from 22 years to 3.

The other integration test is already similar to this: https://github.com/PredictiveEcology/CBM_core/blob/development/tests/testthat/test-3-integration_1-SK-small.R

Why I recommend this: I'm starting to think that with CBM_core, we mostly want integration tests to catch where we may have introduced changes that make the modules incompatible since they were developed to work with it. The other individual modules we are including in the test simulations (e.g. CBM_dataPrep, CBM_vol2biomass, LandRCBM_split3pools) would be responsible for containing more detailed tests checking for specific things such as looking for expected values in the results. I think these checks are more useful there because they exist where the modules are being developed. Aside from major updates, CBM_core should really be the module that is being developed the least frequently.

@DominiqueCaron maybe this integration test with the lengthier validation checks could be a part of LandRCBM_split3pools instead? That way, when it's time to update it to work with CBM4, the tests checks can be updated to fit that new structure as well. For example, the cbm_vars object isn't a part of the CBM4 version of the module.

Copy link
Contributor

@DominiqueCaron DominiqueCaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed!

I believe that the longer simulation time was to test the correct association of cohorts between timesteps after a dispersal event (every 10 years by default in LandR). This test certainly belong more in LandRCBM_split3pools.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants