ncsu-landscape-dynamics / rpops

PoPS (Pest or Pathogen Spread) R Package
https://ncsu-landscape-dynamics.github.io/rpops/
GNU General Public License v3.0
10 stars 5 forks source link

Use PoPS Core with overpopulation movements #83

Closed wenzeslaus closed 3 years ago

wenzeslaus commented 3 years ago

This uses a separate overpopulation_config which is actually passed to the C++ pops_model function.

The config contains three doubles and it is List in C++. I'm not sure if NumericalVector would be more appropriate, however in R it is a List since it has named elements. (In C++ NumericalVector can have named elements, but in R it is coerced to a list AFAIU.)

It could be also a Rcpp nullable parameter, but we pass the boolean elsewhere anyway, so it is boolean plus config.

The translation from individual parameters to the double-only config list is done in the R pops_model function before it is passed to C++ pops_model_cpp function, so pops_model does more than just call the C++ function. It hides the difficulties in calling a C++ function, i.e., the parameter number limit. (Later, pops_model or another function, it could translate from whatever "config" used in R to whatever combo of "configs" the C++ will need.)

ChrisJones687 commented 3 years ago

@wenzeslaus the pops_model r wrapper for the pops_model_cpp function is now merged into master and will make the documentation process easier going forward.

codecov[bot] commented 3 years ago

Codecov Report

Merging #83 (6e6ff6f) into master (d205bac) will decrease coverage by 0.39%. The diff coverage is 70.27%.

:exclamation: Current head 6e6ff6f differs from pull request most recent head 35a2f06. Consider uploading reports for the commit 35a2f06 to get more accurate results Impacted file tree graph

@@            Coverage Diff             @@
##           master      #83      +/-   ##
==========================================
- Coverage   75.57%   75.17%   -0.40%     
==========================================
  Files          40       40              
  Lines        4687     4818     +131     
==========================================
+ Hits         3542     3622      +80     
- Misses       1145     1196      +51     
Impacted Files Coverage Δ
R/auto_manage.R 0.00% <0.00%> (ø)
inst/include/cauchy_kernel.hpp 81.81% <0.00%> (ø)
inst/include/date.hpp 89.36% <ø> (ø)
inst/include/deterministic_kernel.hpp 41.07% <0.00%> (-0.38%) :arrow_down:
inst/include/logistic_kernel.hpp 14.28% <0.00%> (ø)
inst/include/neighbor_kernel.hpp 7.40% <ø> (ø)
inst/include/radial_kernel.hpp 69.35% <0.00%> (ø)
inst/include/simulation.hpp 46.19% <0.00%> (-8.30%) :arrow_down:
inst/include/utils.hpp 64.28% <ø> (ø)
src/pops.cpp 92.39% <33.33%> (-2.15%) :arrow_down:
... and 7 more

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 d205bac...35a2f06. Read the comment docs.

wenzeslaus commented 3 years ago

I have updated the description to reflect the current state.

It has tests in core, but none here yet. Maybe for a different PR just to get things in for further experiments?

@ChrisJones687 Let me know what you think.

wenzeslaus commented 3 years ago

Using List::create() to make the parameter optional causes _"Unable to parse C++ default value 'List::create()' for argument overpopulation_config of function pops_modelcpp". The compilation and tests pass, but according to the documentation ::create() works only for the NumericVector etc. However, input list fails to convert when NumericVector is used as a parameter. The correct solution here is to make it "nullable" with Nullable<List> and set the default to R_NilValue.

The test of use_overpopulation_movements boolean is preserved to keep it explicit (we can consider removing it, but it works in any context, e.g., in pure C++ config in Core). However, the test is extended by config.isNotNull() to check it is not null before accessing it, i.e., it just ignores use_overpopulation_movements that is true if config is null.

There is also isUsable() but I see only isNotNull() being used. The difference is that isNotNull() throws an exception if the variable was not set which would be a programmer error, so it seems like the right function to use.

A bonus of Nullable<List> is that it is similar to C++17 std::optional, so pretty much standard for representing "possibly no value" values in C++.

ChrisJones687 commented 3 years ago

This all looks good. I will add tests for overpopulation to issue #51 for a future PR.