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

Proper generation of param and init lists in TOUCH_FUNS #1013

Merged
merged 5 commits into from Aug 24, 2022
Merged

Conversation

kylebaron
Copy link
Collaborator

@kylebaron kylebaron commented Aug 23, 2022

Summary

In TOUCH_FUNS, we get the param and init slots and Rcpp is able to coerce those to lists when mrgsolve is loaded.

Apparently, when mrgsolve is not loaded, it cannot do this coercion. This was discovered after 1.0.5 was released and @rymarinelli and @kyleam tested reverse dependencies.

I didn't intend to have Rcpp coerce these data structures (it was a coding mistake). So this PR codes correctly: to get the list of parameters or compartment initial values, we need to do two steps:

  1. Get param (or init) slot from the model object
  2. Get data slot from the param (or init) object

When we do this correctly (without relying on Rcpp to coerce), at least one revdep now works.

@kylebaron kylebaron requested a review from kyleam August 23, 2022 21:40
@kyleam
Copy link
Contributor

kyleam commented Aug 23, 2022

I've checked the three MPN packages (mapbayr, campsis, and mrgsim.parallel) with rcmdcheck failures that were likely to be related to this mrgsolve issue. All of them were still failing (though mapbayr was failing with different output).

Perhaps something like this is also needed?

diff --git a/src/devtran.cpp b/src/devtran.cpp
index 1df0c9fd..23e291ad 100644
--- a/src/devtran.cpp
+++ b/src/devtran.cpp
@@ -87,12 +87,14 @@ Rcpp::List DEVTRAN(const Rcpp::List parin,
   request = request - 1;
   
   // Parameters; clone names
-  const Rcpp::List Param = mod.slot("param");
+  Rcpp::S4 paramS4 = mod.slot("param");
+  const Rcpp::List Param = paramS4.slot("data");
   Rcpp::CharacterVector paramnames(Param.names());
   paramnames = Rcpp::clone(paramnames);
   
   // Compartments; clone names
-  const Rcpp::List Init = mod.slot("init");
+  Rcpp::S4 initS4 = mod.slot("init");
+  const Rcpp::List Init = initS4.slot("data");
   Rcpp::CharacterVector cmtnames(Init.names());
   cmtnames = Rcpp::clone(cmtnames);
   Rcpp::NumericVector init(Init.size());

With that on top, all of the package checks pass.

@kylebaron
Copy link
Collaborator Author

@kyleam I fixed the casting that Rcpp was doing in devtran.cpp

Here is the code where we go after slot; most of these are clearly ok; I'll check context on some where it's not clear.

➜  git/m4solve/src (fix/as.list) grep slot *.cpp
LSODA.cpp:  hmax_(Rcpp::as<double>(mod.slot("hmax")));
LSODA.cpp:  hmin_(Rcpp::as<double>(mod.slot("hmin")));
LSODA.cpp:  maxsteps_(Rcpp::as<int>(mod.slot("maxsteps")));
LSODA.cpp:  ixpr_(Rcpp::as<int>(mod.slot("ixpr")));
LSODA.cpp:  mxhnil_(Rcpp::as<int>(mod.slot("mxhnil")));
LSODA.cpp:  rtol_.assign(2, Rcpp::as<double>(mod.slot("rtol")));
LSODA.cpp:  atol_.assign(2, Rcpp::as<double>(mod.slot("atol")));
devtran.cpp:  const double digits = Rcpp::as<double>(mod.slot("digits"));
devtran.cpp:  const double tscale = Rcpp::as<double>(mod.slot("tscale"));
devtran.cpp:  const double mindt  = Rcpp::as<double>(mod.slot("mindt"));
devtran.cpp:  const bool   debug  = Rcpp::as<bool>(mod.slot("debug"));
devtran.cpp:  const bool  verbose = Rcpp::as<bool>(mod.slot("verbose"));
devtran.cpp:  Rcpp::Environment envir = mod.slot("envir");
devtran.cpp:  Rcpp::CharacterVector cap = mod.slot("capture");
devtran.cpp:  Rcpp::IntegerVector capture = mod.slot("Icap");
devtran.cpp:  Rcpp::IntegerVector request = mod.slot("Icmt");
devtran.cpp:  const Rcpp::S4 ParamS4 = mod.slot("param");
devtran.cpp:  const Rcpp::List Param = ParamS4.slot("data");
devtran.cpp:  const Rcpp::S4 InitS4 = mod.slot("init");
devtran.cpp:  const Rcpp::List Init = InitS4.slot("data");
mrgsolve.cpp:  Rcpp::List a = matlist.slot("data");
mrgsolve.cpp:  Rcpp::IntegerVector nn = matlist.slot("n");
odeproblem.cpp:  advan(Rcpp::as<int>(mod.slot("advan")));
odeproblem.cpp:  Rtol = Rcpp::as<double>(mod.slot("rtol"));
odeproblem.cpp:  Atol = Rcpp::as<double>(mod.slot("atol"));
odeproblem.cpp:  ssRtol = Rcpp::as<double>(mod.slot("ss_rtol"));
odeproblem.cpp:  ssAtol = Rcpp::as<double>(mod.slot("ss_atol"));
odeproblem.cpp:  Ss_cmt = Rcpp::as<std::vector<int>>(mod.slot("ss_cmt"));
odeproblem.cpp:  Rcpp::Environment envir = mod.slot("envir");
odeproblem.cpp:  Rcpp::S4 paramS4 = mod.slot("param");
odeproblem.cpp:  Rcpp::List lparam = paramS4.slot("data");
odeproblem.cpp:  Rcpp::S4 initS4 = mod.slot("init");
odeproblem.cpp:  Rcpp::List linit = initS4.slot("data");
odeproblem.cpp:  Rcpp::CharacterVector capture = mod.slot("capture");
odeproblem.cpp:  const Rcpp::S4 omega = mod.slot("omega");
odeproblem.cpp:  const Rcpp::S4 sigma = mod.slot("sigma");

@kylebaron
Copy link
Collaborator Author

These two:

odeproblem.cpp:  const Rcpp::S4 omega = mod.slot("omega");
odeproblem.cpp:  const Rcpp::S4 sigma = mod.slot("sigma");

The S4 object gets passed to MAKEMATRIX which includes

mrgsolve.cpp:  Rcpp::List a = matlist.slot("data");

So that was correct.

Beyond the two spots that were fixed, everything else is atomic or an Environment.

@kylebaron
Copy link
Collaborator Author

The environments don't get used by mrgsolve (they are just for the user to interact with)

devtran.cpp:  Rcpp::Environment envir = mod.slot("envir");
odeproblem.cpp:  Rcpp::Environment envir = mod.slot("envir");

If the revdep checks are now passing as reported by @kyleam hopefully good to go now.

@kyleam
Copy link
Contributor

kyleam commented Aug 24, 2022

@kyleam I fixed the casting that Rcpp was doing in devtran.cpp

Great, thanks for that and for doing a wider scan for possible issues.

I'm verifying that the revdep checks pass with the latest tip (f5c4770).

@kylebaron kylebaron merged commit b2696c4 into develop Aug 24, 2022
@kylebaron kylebaron deleted the fix/as.list branch August 24, 2022 13:44
@kylebaron kylebaron mentioned this pull request Aug 24, 2022
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.

None yet

2 participants