Add test for Coevolve calling user-provided funcs with t beyond tspan

[gaurav-arya]

See #330. The test this PR added was copied into #315, so now this PR just does some minor tweaks.

[isaacsas]

@gzagatti can you give this a review?

My plan is to merge this, make a bug fix release, and then hopefully merge your PR and make a minor release. It would be good after this is merged to update your PR with the test and ensure it doesn't have the same issue.

[gzagatti]

There's a subtle problem with the proposed fix.

Basically, the function next_time is designed to re-use rng in line 168. When you break before computing the next urate, you will be basically re-using the rng in the next interaction which will give incorrect samples. The correct way to proceed is to re-compute urate in those cases rather than recycling the rng.

This is one of the issues addressed by PR #315. In particular see line 260 and line 290. The new implementation of Coevolve ensures that we only compute urate, lrate and rate for the present time t. In other words, it is synced with the model time.

This is so subtle that on my first implementation of PR #315 I could not figure out why some of the samples from Coevolve were slightly different from other algorithms. It was poor recycling of the rng that caused the issue.

With that being said, I think it is valid to add the proposed tests. I would go even further and ensure that lrate, rate and urate are not called beyond tstop. Also you could probably improve the test speed by reducing tstop (or decresing the rates) and setting rateinterval equal to tstop.

[gaurav-arya]

Okay, I've reverted the fix and applied your suggestions to the test (the solve time can be reduced, but note that this will have negligible effect on the test runtime which is nearly all compile time for JumpProblem creation). I think this could be ready to merge as a broken test that your PR would then fix, or we can wait until #315 lands

[isaacsas]

I'm going to merge 315 shortly, and then if you want to tweak the version of this test here just update this PR and I'll merge it.

Thanks!

[isaacsas]

version of the test "here" I mean...

[coveralls]

Pull Request Test Coverage Report for Build 5537683364

Warning: 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.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+0.004%) to 88.452%

---

Files with Coverage Reduction	New Missed Lines	%
src/aggregators/coevolve.jl	1	98.41%
<!--	Total:	1		-->

Totals
Change from base Build 5520199253:	0.004%
Covered Lines:	2183
Relevant Lines:	2468

---

💛 - Coveralls