AbstractSSAJumpAggregator interface #20

alanderos91 · 2018-04-04T03:33:14Z

I think everything we discussed is in place. Please feel free to suggest alternative names for the various functions and fields added.

This couples everything to an aggregator. One can overload these if desired.

Default to xorshifts to make simulation multithreading-safe.

The template can be re-used by other aggregators. Someday we should be able to use the generic routines pending: JuliaLang/julia#14919

ChrisRackauckas · 2018-04-04T03:44:33Z

@isaacsas wanna take a look?

ChrisRackauckas · 2018-04-04T03:49:59Z

src/problem.jl

@@ -19,15 +19,16 @@ JumpProblem(prob,aggregator::AbstractAggregatorAlgorithm,jumps::AbstractJump...;
 JumpProblem(prob,jumps::JumpSet;kwargs...) = JumpProblem(prob,NullAggregator(),jumps;kwargs...)

 function JumpProblem(prob,aggregator::AbstractAggregatorAlgorithm,jumps::JumpSet;
-                     save_positions = typeof(prob) <: AbstractDiscreteProblem ? (false,true) : (true,true))
+                     save_positions = typeof(prob) <: AbstractDiscreteProblem ? (false,true) : (true,true),
+                     rng = Xorshifts.Xoroshiro128Star(rand(UInt64)))


Let's use Xoroshiro128Plus. See the end of the chat which states it's not the same as the one in the post.

I thought Xoroshiro128Plus is claimed to fail particular test battery? I'm using Xoroshiro128Star.

No, XorShift128Plus, not Xoroshiro128Plus.

Just to clarify. The blog post found issues with Xoroshiro128Plus. It recommended as fine XorShift* 128/64. I haven't found any info about Xoroshiro128star.

Oh really? But then it passed all of the tests of the repo?

It failed a different series of tests (newer I think) called PractRand, and another series called TestU01. These are different from the Big Crush tests (which are the most popular I think). I'm not an expert on these tests, so don't know the significance, but at least some people seem to feel that there are good generators that don't fail these tests that should be preferred. You can see the details on the blog post I linked back in the issue thread.

I don't know enough to comment on this but can change as needed.

ChrisRackauckas · 2018-04-04T03:55:41Z

Forgot to define the abstract type

isaacsas · 2018-04-04T04:35:47Z

src/aggregators/direct.jl

+  rates = p.rates
+
+  @inbounds fill_cur_rates(u, params, t, cur_rates, 1, rates...)
+  @inbounds cur_rates[1] = cur_rates[1]


Is this necessary?

codecov · 2018-04-04T04:36:15Z

Codecov Report

Merging #20 into master will decrease coverage by 0.34%.
The diff coverage is 97.5%.

@@            Coverage Diff             @@
##           master      #20      +/-   ##
==========================================
- Coverage   78.73%   78.39%   -0.35%     
==========================================
  Files          13       14       +1     
  Lines         428      435       +7     
==========================================
+ Hits          337      341       +4     
- Misses         91       94       +3

Impacted Files	Coverage Δ
src/DiffEqJump.jl	`100% <ø> (ø)`	⬆️
src/aggregators/direct.jl	`100% <100%> (ø)`	⬆️
src/aggregators/ssajump.jl	`100% <100%> (ø)`
src/problem.jl	`68.57% <50%> (ø)`	⬆️
src/coupling.jl	`73.62% <0%> (-3.3%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd4479e...0003134. Read the comment docs.

isaacsas · 2018-04-04T04:36:40Z

src/aggregators/direct.jl

+############################# Required Functions #############################
+
+# creating the JumpAggregation structure
+@inline function aggregate(aggregator::Direct, u, p, t, end_time, constant_jumps, save_positions, rng)


The inline here also seems unnecessary (it was present before but I don't see what good it can do here).

Yeah it's probably unnecessary.

coveralls · 2018-04-04T04:38:28Z

Coverage decreased (-0.3%) to 78.391% when pulling 0003134 on alanderos91:master into cd4479e on JuliaDiffEq:master.

isaacsas · 2018-04-04T04:51:15Z

It looks good to me modulo the issue about Float64's in aggregate. This is the basic interface we discussed in the issue thread. I'll send through the mass action updates once this is in. Thanks @alanderos91!

isaacsas · 2018-04-04T04:56:50Z

src/aggregators/direct.jl

+function aggregate(aggregator::Direct, u, p, t, end_time, constant_jumps, save_positions, rng)
+  rates = ((c.rate for c in constant_jumps)...)
+  affects! = ((c.affect! for c in constant_jumps)...)
+  cur_rates = Vector{Float64}(length(rates))


Maybe instead of using Float64's here you should use typeof(t)?

Sure. I'll hold off on pushing so I don't trigger so many builds...

Yeah, sorry for the intermittent stream of comments there. I'm still getting used to looking at PRs on Github... That should be it from me -- I've gone through everything now.

Don't worry about triggering builds. It'll queue.

Note you can also add [ci-skip] to make it not queue a build for a commit.

alanderos91 · 2018-04-04T05:34:04Z

Fantastic. Many thanks to both you for all the help! 🎉

ChrisRackauckas · 2018-04-04T05:44:21Z

Many thanks to you!

alanderos91 added 8 commits April 2, 2018 13:28

Add common interface for SSA algorithms

9490b16

Use the SSA interface and rewrite Direct accordingly

b1d2699

Inline helper functions

99761bf

Rewrite helper functions for sampling

7db21b9

This couples everything to an aggregator. One can overload these if desired.

Use RandomNumbers for RNG

11320e0

Default to xorshifts to make simulation multithreading-safe.

Rewrite the interface to provide a better template

a8fc273

The template can be re-used by other aggregators. Someday we should be able to use the generic routines pending: JuliaLang/julia#14919

Use Xoroshiro128Star

267fa8b

Rename to AbstractSSAJumpAggregator

8843e2d

ChrisRackauckas reviewed Apr 4, 2018

View reviewed changes

Fix typo

568e568

isaacsas reviewed Apr 4, 2018

View reviewed changes

Remove uneccessary @inline

4b3d546

Remove dangling lines

235c2cb

isaacsas reviewed Apr 4, 2018

View reviewed changes

Don't specify Float64 [ci-skip]

0003134

ChrisRackauckas merged commit 13df478 into SciML:master Apr 4, 2018

alanderos91 mentioned this pull request Apr 5, 2018

Proposal: Abstraction for joint distribution sampling #19

Closed

ChrisRackauckas mentioned this pull request Apr 5, 2018

Inference stops at 10+ jumps SciML/DifferentialEquations.jl#270

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AbstractSSAJumpAggregator interface #20

AbstractSSAJumpAggregator interface #20

alanderos91 commented Apr 4, 2018

ChrisRackauckas commented Apr 4, 2018

ChrisRackauckas Apr 4, 2018

alanderos91 Apr 4, 2018

ChrisRackauckas Apr 4, 2018

isaacsas Apr 4, 2018

ChrisRackauckas Apr 4, 2018

isaacsas Apr 4, 2018 •

edited

Loading

alanderos91 Apr 4, 2018

ChrisRackauckas commented Apr 4, 2018

isaacsas Apr 4, 2018

codecov bot commented Apr 4, 2018 •

edited

Loading

isaacsas Apr 4, 2018

ChrisRackauckas Apr 4, 2018

coveralls commented Apr 4, 2018 •

edited

Loading

isaacsas commented Apr 4, 2018 •

edited

Loading

isaacsas Apr 4, 2018

alanderos91 Apr 4, 2018

isaacsas Apr 4, 2018

ChrisRackauckas Apr 4, 2018

ChrisRackauckas Apr 4, 2018

alanderos91 commented Apr 4, 2018

ChrisRackauckas commented Apr 4, 2018

AbstractSSAJumpAggregator interface #20

AbstractSSAJumpAggregator interface #20

Conversation

alanderos91 commented Apr 4, 2018

ChrisRackauckas commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaacsas Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisRackauckas commented Apr 4, 2018

Choose a reason for hiding this comment

codecov bot commented Apr 4, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 4, 2018 • edited Loading

isaacsas commented Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanderos91 commented Apr 4, 2018

ChrisRackauckas commented Apr 4, 2018

isaacsas Apr 4, 2018 •

edited

Loading

codecov bot commented Apr 4, 2018 •

edited

Loading

coveralls commented Apr 4, 2018 •

edited

Loading

isaacsas commented Apr 4, 2018 •

edited

Loading