-
-
Notifications
You must be signed in to change notification settings - Fork 185
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
CVODES #262
Comments
I think Michael's going to have to chime in on the math and algorithms. Is that PERC model available somewhere we can use to benchmark? Does anybody know if we can distribute the data? Otherwise, maybe Michael has some ideas for performance tests? I'm more than happy to help out with the nitpicky code integration details. |
Hi! Thanks Bob for supporting! I am asking Frederic about the PERC model right now, will let you know once I have feedback from him. I can provide other large (published) models which show the same problems when being solved with cvode+coupled instead of CVODES. In case we want to merge in smaller steps, then we could consider merging the current branch feature-262-CVODES. Right now all I have done is to replace CVODE with CVODES. Since CVODES is 100% compatible to CVODE, there is no code change right now to the current CVODE+coupled code and all tests do pass! In case we want to swap out CVODE for CVODES now, then I would branch of this branch in order to include the really big change this switch brings. Just let me know what is best here... otherwise I proceed an populate this feature branch with my code. |
Sebastian, thanks for asking Frederic about it.
On Fri, Mar 18, 2016 at 9:39 AM, wds15 notifications@github.com wrote:
|
for the purpose of an ODE benchmark, we don't need the data. It would be nice, yes; but if we only are out for the ODE performance, then the ODE and a parameter set are sufficient. And as I said, if we don't trust the PERC model, I can provide other (published) examples. Let me prepare one of those. I take that we do this in a single go (or cherry pick later smaller part of this branch). Great. |
Thanks. Please provide another example until we can get the perc model I didn't understand your last comment. On Fri, Mar 18, 2016 at 9:53 AM, wds15 notifications@github.com wrote:
|
I think I found a great example: The Hornberg model is described in the reference DOI: 10.1111/j.1432-1033.2004.04404.x is a great case. Unfortunately the paper itself is paywalled, but the model itself is freely available here (this link is right next to the abstract): http://jjj.biochem.sun.ac.za/database/hornberg/index.html This model has 8 states+18 parameters which describe a system of reactions with a number of michaelis-menten kinetics. This is a large, but very typical use-case I suppose. The model one can download includes the equations, initial values and parameters to simulate. I suggest to put this into an executable test which I propose to put on an independent branch such that we can merge this into develop eventually and also merge this into the CVODES branch. I hope that makes sense. |
Sounds great.
|
Ok, here it goes! The stiff stress test is taking ages with the old cvode implementation. The same test is now also included in the CVODES branch which I just pushed to the repository. The test can be triggered with /runTests.py test/unit/math/rev/arr/functor/integrate_ode_perf_test.cpp The CVODES branch currently passes all integrated_ode_cvode* tests. However, the coupled_ode_system_cvode* tests do NOT pass at the moment as I have replaced the coupled_ode_system_cvode with the cvodes_integrator object. From what I can see, it would make sense to change the coupled_ode_system_cvode tests such that they test instead the cvodes_integrator. I am happy to do that. However, I guess first we should settle on the design of things as I did change a few things. Most importantly, I do not anymore use a shifted system when initials vary. Moreover, the code I supply can serve as a refactoring base for the odeint based integrator. Finally, I am also providing another hornberg test which demonstrates how one can use partial template specialization in order to provide analytic Jacobians. |
I have ported all cvode related tests to the new code. I renamed in the folder test/unit/math/rev/arr/functor/
and ported each test to the slightly updated logic. I suppose these two files are a good reference to demonstrate how the cvodes integrator works. ALL cvode related tests have now been ported to the new system and ALL tests do pass. The source code contains at the moment here and there still some comments to make it easier to grasp what changed how. For the final merge we can delete a number of those comments. Moreover, I would like to enable the non-stiff solver from cvode such that I would propose to add a further "solver" argument to the integrate_ode_cvode function. The code is prepared for that, but I am not sure if you agree on this. |
That all sounds great. @betanalpha, can you weigh in on this? @wds15: That all sounds great! When you say you don't use a @betanalpha: can you take a look at this and weigh in on any
|
We can add a new function, but we don't want to break the integrate_ode_cvodes(...); and then our existing inegrate_ode(...) can delegate to one of the versions. From the semantic side Others may have better ideas for naming. We can deprecate the old function and get rid of it in |
I am definetly calculating the very same function, of course. In fact, all the dy_dt's are exactly the same, the only difference is how the initials are set. The test file cvodes_integrator_rev_test.cpp shows how this differs. I am proposing to leave the integrate_ode function just as it is. All I am suggesting is to change the function signature of integrate_ode_cvode , i.e. I propose to add a further "solver" argument which I suggest to be an integer. To my knowledge the function integrate_ode_cvode was never released, but only "lived" on the develop branch. This is why I thought changing the signature at this stage would be ok. Specifically I would like to have the solver definitions to be
The STaLD algorithm is a stability limit detection algorithm for BDF. It does cost you 5-7% performance and is in rare cases (according to the documentation of CVODES) needed in order to guarantee numerical stability of the BDF method. In case I manage to figure out how to compile CVODES with the MKL, then we could add another stiff solver which uses the MKL - given this is noticeable in speedup and can be easily deployed. To proceed, should I kick off a pull request? |
I think we need to step back and more carefully understand some underlying issues.
On Mar 20, 2016, at 6:47 PM, Bob Carpenter notifications@github.com wrote:
|
Hi!
|
|
.. and to add to my previous answer: CVODES is CVODE with Sensitivity support. So this is an extension to CVODE which specifically adresses sensitivity stuff. There has to be a reason why sundials provides a specific software suite for ODE+Sensitivity problems and the ODE stiff benchmark is an example of this. So, yes, using CVODES gives such a large speedup not only due to better memory locality, but mostly due to better algorithms to solve the ODE+sensitivity problem. |
I am here referring to the former and how accurate we need the numerical solutions to be before
|
|
No, no, no. I’m saying that the absolute and relative sensitivities that specify On Mar 21, 2016, at 12:49 PM, wds15 notifications@github.com wrote:
|
Well, CVODES offers more control over the absolute and relative tolerances for the sensitivities. In fact, if you want to, you can set these for each parameter sensitivity separatley. So CVODES should let us tune things to higher accuracy. Although, I have to say that I don't yet fully understand why such high accuracy sensitivites are needed. Do you have examples somewhere which show what goes wrong? I mean, I am getting very good inferences from Stan when using ODEs. Maybe things are not ultra-precise, but in my applications the accuracy is sufficient enough. At least from what I can say. But to come back to CVODES: The implementation passes all tests which were written for CVODE; what else is needed to merge this? The increased accuracy story is separate to this and should be dealt with in another issue+pull request, no? |
I AM NOT TALKING ABOUT JUST THE ACCURACY OF THE SENSITIVITIES. Would everyone be happy if our exp or log implementations had absolute accuracy This point is relevant to this and the other PR because they are based on On Mar 21, 2016, at 1:39 PM, wds15 notifications@github.com wrote:
|
No one expects ODEs to be so super-precised solved. If that matters for NuTS, ok then this is a different story. Of course, we have to define at what tolerances we want comparisons to be run. I followed your suggestion of putting 1E-10 abs+rel tolerances in the test case I provided. The old CVODE+coupled system does fail here again while the CVODES implementation passes this test. |
Hi! I added a further performance test case, i.e. this time one which is possibly a minimal example. The ODE model consists of 2 states and 4 parameters only. Essentially, one state becomes activated and then becomes deactivated again. Both reactions are governed by a michaelis menten type rate . The ODE system is
Moreover, I increased absolute and relative tolerance to 1E-10. The new smaller test case does again fail with the old cvode+coupled system which manages to integrate 24 times the system within a minute time. The new CVODES system instead runs 100 integrations in 0.1s. Michaelis-Menten ODEs are a primary application area for the ODE solver in Stan. At least for my application purposes. |
My guess is that while 1e-5 may be a problem, I doubt 1e-7 is going I may be way off base, of course!
|
The problem is that the ODE errors can potentially aggregate coherently over multiple time @wds15, here is what I think needs to be done before a pull request.
1a) Take a few ODE models that span typical applications, including one exact model 1b) Run with CVODE (or CVODES if you prefer) with very small rel and abs tolerances 1c) Now scan through abs/rel tolerances, say jumping 2 orders of magnitude at each Use the states vs time to quantify the actual error in the ODE solver for each setting. Plot actual error vs abs/rel tolerance and the mean/variances/standard errors vs 1d) We should hopefully see a cut off where the abs/rel tolerance is low enough that Then we�ll be in a position to properly judge new samplers.
On Mar 22, 2016, at 8:03 PM, Bob Carpenter notifications@github.com wrote:
|
I understand what (and why) you want to test. So I think
|
Right. I have no objections to putting CVODES in On Mar 22, 2016, at 9:56 PM, Bob Carpenter notifications@github.com wrote:
|
Do these tests already exist for the first two solvers
|
Yes, although with somewhat arbitrary accuracy thresholds. On Mar 22, 2016, at 10:06 PM, Bob Carpenter notifications@github.com wrote:
|
I fully agree that a good understanding of ODE estimation error and MCMC error is very valuable. However, I have to say that I doubt that we can ever cover the space of models/parameters reasonably well as there are too many unkowns (and unknown unkowns). That said, I can use a program I have written a while ago to look a bit into this. This problem is a hierarchical PK/PD problem where I simulate from some true values and then use a Stan program which I can switch between using an analytic solution OR the ode integrator. I guess this would be a good base to do such a study - and I am definetly myself very interested in such results as I am interested in the question of how low I can set the precision and still be safe. For the stiff ODE performance tests I am not so sure if we need to tune this to ultra-high precision. I mean these are tough problems and I am happy to live with a somewhat biased solution rather than none. I know that I am coming here a bit from the other side yes, and I do agree that understanding bias also in these settings is valuable, I just doubt it will be practical to insist on ulta-high precision (although I have set CVODES to 1E-10 which should be a "true" 1E-8). I guess you speak of the *_grad_test.cpp tests which do test at precisions of 1E-8 and 1E-7. To me that sounds enough, but for the moment we could just bump that to a higher precision (1E-10?) and possibly later revise this setting after all those tests? I mean, the benefit of CVODES can be very large for our users. |
You are completely speculating here. If we’re going to be comparing On Mar 23, 2016, at 10:41 AM, wds15 notifications@github.com wrote:
|
The fact that we can't cover every contingency with a suite I believe all that Michael's asking for here is to show that Even if we weren't comparing CVODES, we'd want these tests Given that we will have user-settable precision, the decision
|
Yes, as I said, I can run those tests on a model I have prepared. All simulated stuff and direct comparison with an analytic solution, I guess this is the best comparison we can do. Agree? Given that we consider these tolerances as very important, I could enable the possibility to set an absolute tolerance PER state within CVODES (which will appropiatley propagate to the sensitivities). Does that make sense? So we would have two functions, one taking a vector of abs_tols and one taking just a real which gets applied to all states. Then the user can decide for which state he is willing to pay a large performance price or not. This is recommended practice from the CVODES user manual. Wrt. to precisions: There are tests in Stan which are set to 1E-7 and 1E-8 right now and CVODES does pass these, just as CVODE does. To expedite this pull request I was suggesting to upping that to 1E-10 for now... higher precisions will anyway be a problem for about any solver as the double machine precision is around 1E-14. |
I was very clear in my original email what we need to first see. On Mar 23, 2016, at 4:20 PM, wds15 notifications@github.com wrote:
|
Hi! Not sure where to put this, but I don't want to spam the dev list (maybe I should?) with this. So here are the results for a hierarchical PK/PD model. To be specific, I simulated 50 patients of a 2-cmt model oral dosing situation for typical parameter values I use to see. Two doses (t=0 and t=24) are given and I compare against an analytic solution which I placed on the very left of the plots with abs_tol=1E-16 for machine precision. Now, I had to use the CVODES implementation as the CVODE+coupled did freeze Stan for 3 days while CVODES returned the results in few hours, depending on the abs+rel tolerance set. I used a 1E-9, 1E-6 and a 1E-3 abs+rel tolerance. Have a look yourself, but I can't find any threshold. I used 4 chains and per chain I did 500 warmups and 500 iterations. True values are
|
So all tests are brought in line with at least 1E-6 abs+rel tolerance and I added a integrate_ode_cvodes function which takes a solver argument. Of course, we can quickly adapt to whatever naming convention we end up with (probable the _stiff naming). @bob-carpenter , @betanalpha a code review from one of you would be great. At the moment a high-level feedback would be very useful for me to shape this thing up for a pull request. There are still cpplint errors... will attack them soon. |
I can do a basic code review tomorrow. Michael will be
|
Summary:
replace cvode integrator with codes. So remove cvode and replace functionality by cvodes.
Description:
The current cvode integrator uses a coupled ode system in order to obtain sensitivities of an ODE system. This is handled in a fully integrated way by cvodes. Benefits of using cvodes:
Reproducible Steps:
Bad performance with big ODE systems. For example, the PERC model. Other big models are equally suitable to show slow performance of the cvode/coupled system.
Current Output:
Some ODE systems make the cvode/coupled integrator very slow.
Expected Output:
Large ODE systems should not slow down the integrator.
Additional Information:
This is a large patch and we may just use this as a basis to split into smaller bits and pieces. This patch introduces also the ode_model object which can be used to enable analytic Jacobians for ODE systems via template specialization. Moreover, the decoupling operation is split out of the coupled_ode system object. This patch can form the basis for a nice refactoring of the coupled_ode_system.
Current Version:
None. Only the current develop version of math
The text was updated successfully, but these errors were encountered: