-
-
Notifications
You must be signed in to change notification settings - Fork 35
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
CoevolveSynced #315
CoevolveSynced #315
Conversation
It seems to indicate that you want to update the rate function according to the previous (stochastic ) history of the current trajectory. Is it what you want to do in the end? |
Pull Request Test Coverage Report for Build 4773569602Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
What do you mean by the "previous (stochastic ) history of the current trajectory"? Is that vector
Does it make sense now? |
With the goal of developing a new benchmark for jump problems coupled with continuous ones, I have started working on the synapse model discussed in this Discourse thread. You can find a discussion thread with regards to this development in rveltz/SynapseElife.jl#1. In particular, I have raised some questions about the best design pattern for this sort of problem in my latest comment. @isaacsas and @ChrisRackauckas when and if you have time please help with your inputs. |
Don't worry about the formater failing. There was a style change so almost every file will need updating. I don't want to mix those changes up with your PR... Going to take a last pass through the PR this afternoon, and then hopefully we will be good to go (modulo the comment I made on the other PR about that test/issue). |
Ok so three issues to settle:
|
Co-authored-by: Sam Isaacson <[email protected]>
Co-authored-by: Sam Isaacson <[email protected]>
With regard to your points:
save_everystep = any(cb.save_positions) Also, I don't see in the stepper code an instruction to save before jumps. From my understanding the code only saves after the jump even if With In this case, the """Whether to save every time a jump occurs."""
save_everystep::Bool I was not that comfortable with modifying the stepper. You might have a better idea on how to handle all the saving corner cases, so please let me know how to proceed.
|
1 isn't just about saving. For 3. if that PR would result in an incorrect algorithm, and it would be substantial work to fix it, I guess it would be better to just add an analogous test here and make sure the issue is fixed here. I would've liked to release a fix on the older Coevolve for anyone actively using it, but if it would be a major PR to fix it then probably it makes more sense to just merge this PR and release. |
I didn't quite understand your last point on the interpolation. However, my understanding is that
|
For an ODE time stepper |
Thanks for clarifying on the interpolation. It's much clearer now. I think it might be worth adding some explanation about it on the documentation. Users might indeed be expecting a behavior similar to the ODE case, such that |
I have pushed the changes to the saving behavior that you originally suggested above. In addition to that, I have included some documentation on it to guide library users. I'm glad we had the discussion above. Thanks to it, the saving behavior of the stepper is much clearer to me. I hope that's all you need to get the PR merged. |
@gzagatti all merged, thanks for all your efforts on this! |
@isaacsas it's always a pleasure working with you. I learn a lot! |
This PR introduces a new aggregator
CoevolveSynced
.A known drawback of
Coevolve
is that it requires to look ahead during the thinning routine. This limits the interoperability ofCoevolve
withODEProblem
s. It turns out that we can sync the thinning routine with the stepper which removes the need to look ahead. The only requirement is that the local bound of the rate computed at timet
givenu
be valid fromt
tot + rateinterval
which is a very mild requirement.The PR modifies
Coevolve
as following. Instead of computing the next jump time, it only computes the next candidate time. The candidate is then pushed to the priority queue which is passed to the stepper. The stepper steps at the earliest candidate time and determines whether this time should be accepted or rejected. If the time is rejected, the jump is not computed. Otherwise, the jump is recorded and dependencies are updated. This procedure should be equivalent toCoevolve
.The algorithm only modifies the handling of
VariableRateJump
while the other jumps are handled the same way as inCoevolve
. Despite of that, there seems to be a small penalty asCoevolveSynced
runs slightly slower thanCoevolve
when noVariableRateJump
s are involved. I am wondering if there is some obvious efficiencies that I have missed out.Below you can find the
SciMLBenchmarks
:Diffusion model
Mendes multivariate
Negative feedback gene expression model
Negative feedback Marchetti model
Multivariate Hawkes model
In terms of correctness,
CoevolveSynced
produces similar statistics toCoevolve
and QQ-plots are comparable as you can see below:QQ-plots Multivariate Hawkes model
While we review this PR, I would like to benchmark
CoevolveSynced
withPDMPCHV
for the synapse model discussed in this Discourse thread which is of very practical value.