Closed aornugent closed 2 years ago
This still requires a little house keeping, including updating documentation and some R functions as you note above.
The simplified FF16_Environment constructors work well and the differences between strategies are easily handled in the R API, with flexibility for users to override the defaults as needed.
Only canopy_rescale_usually
and soil_number_of_depths
are fixed/read-only. Environment rescaling only takes place in a Patch, so it may be better as a control parameter instead. I also introduced some auxilliary functions to separate environment from cohort ODE sizes (L41 of patch.h, L59 of test_patch) - this might be extended to ODE states and rates in a later PR.
Will notify once final touches have been made, but please feel free to test and review whenever convenient.
:exclamation: No coverage uploaded for pull request base (
develop@195c1c5
). Click here to learn what that means. The diff coverage isn/a
.
@@ Coverage Diff @@
## develop #315 +/- ##
==========================================
Coverage ? 79.92%
==========================================
Files ? 94
Lines ? 8459
Branches ? 0
==========================================
Hits ? 6761
Misses ? 1698
Partials ? 0
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 195c1c5...8bd8f15. Read the comment docs.
Hi @aornugent
This is overall really good.
But still an important unresolved point above: https://github.com/traitecoevo/plant/pull/315#discussion_r707788734
We should remove default type for "make_environment()" and "scm_base_control()", and draw these from the parameters object, using something like function(p, env = make_environment(p$type), ctrl = scm_base_control(p$type))
.
Otherwise it will be not very user friendly for any strategies other than "FF16"
Another incremental change toward #306.
This removes the Environment object from Parameters and passes it directly to SCM or Patch APIs as required. This is handled on the R side with what I hope are sensible defaults, but should be straightforward for users to configure.
The major motivation for this was to have strategy specific variants of the FF16 Environment. See
ff16_environment.cpp
andplant::Water_make_environment
for an example (noting that Water -> FF16w in the future).It's tripped up something in the Stochastic Patch which isn't initialising the cohort introduction schedule correctly. Will continue to investigate. Will rebase off develop once the issue has been resolved and #314 has been merged.