-
-
Notifications
You must be signed in to change notification settings - Fork 188
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
Variadic argument lists for ODEs #1641
Conversation
…into proto-parallel-v3
…rograms to handle general case
…hat I want it to look like
…hat I want it to look like
…hat I want it to look like
…hat I want it to look like
I just ported the SIR example to use variadic and I am running it. Observations:
We really need to motivate people to switch with 2.24+ to use variadic ODE... or they pay 30% performance penalty. However, rewriting the models like this is almost a joy to do since it is so much more natural to write For reference, here is the SIR variadic model: // Simple SIR model inspired by the presentation in
// http://www.ncbi.nlm.nih.gov/pmc/articles/PMC3380087/pdf/nihms372789.pdf
functions {
// beta, water contact rate
// kappa, C_{50}
// gamma, recovery rate
// xi, bacteria production rate
// delta, bacteria removal rate
vector simple_SIR(real t,
vector y,
real beta,
real kappa,
real gamma,
real xi,
real delta) {
vector[4] dydt;
dydt[1] = - beta * y[4] / (y[4] + kappa) * y[1];
dydt[2] = beta * y[4] / (y[4] + kappa) * y[1] - gamma * y[2];
dydt[3] = gamma * y[2];
dydt[4] = xi * y[2] - delta * y[4];
return dydt;
}
}
data {
int<lower=0> N_t;
real t[N_t];
vector[4] y0;
int stoi_hat[N_t];
real B_hat[N_t];
}
transformed data {
real t0 = 0;
real<lower=0> known_kappa = 1000000.0;
}
parameters {
real<lower=0> beta;
real<lower=0> gamma;
real<lower=0> xi;
real<lower=0> delta;
}
transformed parameters {
real kappa = known_kappa;
vector<lower=0>[4] y[N_t] = ode_rk45(simple_SIR, y0, t0, t, beta, kappa, gamma, xi, delta);
}
model {
real y_hat[N_t];
beta ~ cauchy(0, 2.5);
gamma ~ cauchy(0, 1);
xi ~ cauchy(0, 25);
delta ~ cauchy(0, 1);
stoi_hat[1] ~ poisson(y0[1] - y[1, 1]);
for (n in 2:N_t)
stoi_hat[n] ~ poisson(y[n - 1, 1] - y[n, 1]);
for (n in 1:N_t)
y_hat[n] = y[n,4];
B_hat ~ lognormal(log(y_hat), 0.15);
} I made |
… catch it (design-doc #19)
…-dev/math into feature/parameter-pack-odes
Good catch. Fixed and changed the tests to catch this here |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Do we need a "Release Notes" section for this PR? I would say no, since we will anyway feature the variadic ODE boldly in the release notes. |
We can do that. We won't forget to mention this one. Should we just merge this? There will be a conflict with #1798 The stanc3 one is also close. |
I think we are ready to merge. I would merge once we are confident that develop will stay "green" given that stanc3 must go in and the stan repo may need to switch to stanc3. |
The old interface is the same as it was and there are currently no models with the new interface in the pipeline so no reason anything would fail. Stan repo now uses stanc3 though that is also not required now that the old interface uses the old functor. The tests that went green here have no special branches set. So merge away. |
Great... if that's the case then we can merge. Maybe we let @bbbales2 the pleasure of doing that? |
I'm gonna do #1798 first since that'll let me simplify this. Should be able to get this green again by the end of the day. |
That was an easier merge than I expected. This'll need approved again (here). Should be good to go once things are green. |
@serban-nicusor-toptal the asnyc OpenCL are commented out now until we figure out what is going on with the gelman-group-linux setup. Just a heads up that you are aware. |
Should we merge without that async GPU tests? Or what is the plan? |
Yes, if possible that we do that here. I can also open a separate PR to do only the comment-out change and then once that is merged we re-run this. I can do that if you would prefer. That async tests should not run anyways on this PR, as this PR does not touch the /opencl folder. But I think due to the merge of recent develop Jenkins now thinks that there were changes there. I double checked that there are no changes there. The problem is that for some reason gelman-group-linux is acting weirdly, cant find the GPU and also some header file problems with compiling TBB on distribution tests. |
I am fine with commenting out the OpenCL stuff here on this PR... maybe create an issue for sorting out the issues so that we track this? |
Good call. Reopened #1940 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
bump... @bbbales2 merge ?! |
Summary
See issue #1642.
See design here: stan-dev/design-docs#20
Edit: Updating description to latest changes (April 13th, 2020)
There are four new functions here:
I switched to shorter names because typing
integrator
every time seems unnecessary. It has the bonus effect of the old interfaces can remain like they were.The
_tol
versions of the functions accept the relative tolerance, absolute tolerance, and max number of iterations arguments.The interface is meant to looks like the example in the issue:
and
where the ode right hand side function looks like:
The old interfaces (
intergrate_ode_bdf
andintegrate_ode_rk45
) are meant to work as they did previously (they just wrapode_bdf_tol
andode_rk45_tol
now).C++ Notes
The variadic arguments need to come last in the ode rhs functor. Something like:
This means all the existing ode rhs functors need changed even though the stan interface to
integrate_ode_rk45
andintegrate_ode_bdf
aren't changing. Something like this would work:The old interface looked like:
If this is a problem (which I assume it is for Math library versioning cuz this would break code), I believe we can add an adapter interface inside the new placeholder
integrate_ode_*
functions. This would mean doing code generation in Stan conditional on what function is using the functor which isn't ideal.I removed the
coupled_ode_observer
class. That functionality is now split betweenfun/ode_store_sensitivities.hpp
andfunctor/ode_add_time_gradients
(both of these files are used in the boost and cvodes integrators).I removed the
coupled_ode_data
class. Most of that just lives in the class inrev/cvodes_integrator.hpp
now.functor/coupled_ode_system.hpp
has lots of changes. Generally I was trying to simplify that class.Tests
This passes the existing tests, but will need more cause it supports a lot more stuff.
Side Effects
Did a lot of reorganizing the ODE stuff. All sorts of things could be broken.
Checklist
Math issue Variadic argument lists for ODEs #1642
Copyright holder: Columbia University
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
./runTests.py test/unit
)make test-headers
)make doxygen
)make cpplint
)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested