Refactor environment

[aornugent]

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.

result <- run_scm_collect(p = expand_parameters(p0, trait_matrix), 
                          env = make_environment("FF16"),
                          control = scm_base_control())

The major motivation for this was to have strategy specific variants of the FF16 Environment. See ff16_environment.cpp and plant::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.

[dfalster]

Nice work @aornugent . Some suggestions included.

[dfalster]

Is there a reason to comment it out instead of delete? This will leave us wondering why it's commented out.

[aornugent]

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.

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (develop@195c1c5). Click here to learn what that means.
The diff coverage is n/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.

[dfalster]

Hi @aornugent

This is overall really good.

But still an important unresolved point above: #315 (comment)

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"

[aornugent]

@devmitch @dfalster - the unsatisfying solution was to hard code the arguments for K93_Environment. This model is only really for demonstration purposes, so the issue may rear its head again in the future.