-
-
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
Add test for Coevolve calling user-provided funcs with t beyond tspan #331
Conversation
@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. |
There's a subtle problem with the proposed fix. Basically, the function 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 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 |
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 |
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! |
version of the test "here" I mean... |
Pull Request Test Coverage Report for Build 5537683364Warning: 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 |
See #330. The test this PR added was copied into #315, so now this PR just does some minor tweaks.