Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use PoPS Core with overpopulation movements #83

Merged
merged 13 commits into from
Apr 8, 2021

Conversation

wenzeslaus
Copy link
Member

@wenzeslaus wenzeslaus commented Mar 12, 2021

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

@wenzeslaus wenzeslaus added the enhancement New feature or request label Mar 31, 2021
@ChrisJones687
Copy link
Member

@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
Copy link

codecov bot commented Apr 5, 2021

Codecov Report

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

❗ 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%) ⬇️
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%) ⬇️
inst/include/utils.hpp 64.28% <ø> (ø)
src/pops.cpp 92.39% <33.33%> (-2.15%) ⬇️
... 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 wenzeslaus marked this pull request as ready for review April 6, 2021 13:41
@wenzeslaus
Copy link
Member Author

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
Copy link
Member Author

Using List::create() to make the parameter optional causes "Unable to parse C++ default value 'List::create()' for argument overpopulation_config of function pops_model_cpp". 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
Copy link
Member

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

@ChrisJones687 ChrisJones687 merged commit c3903d7 into master Apr 8, 2021
@ChrisJones687 ChrisJones687 deleted the add-overpopulation-pest branch April 8, 2021 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants