DirectCR-RSSA #346

Vilin97 · 2023-09-11T04:39:20Z

A new spatial SSA calledDirectCRRSSAuses RSSA-style sampling to sample the site+jump. It is about 20% faster thanDirectCRDirect,at least on the problem in thetest/spatial/ABC.jl(5 sites in a line, 3 species, 2 reactions).

using BenchmarkTools
@btime solve(jump_problems[1], SSAStepper()) # NSM with a `CartesianGrid`, 52ms
@btime solve(jump_problems[2], SSAStepper()) # NSM with a `Graph`, 49ms
@btime solve(jump_problems[3], SSAStepper()) # DirectCRDirect with a `CartesianGrid`, 51ms
@btime solve(jump_problems[4], SSAStepper()) # DirectCRRSSA with a `CartesianGrid`, 42ms

github-actions · 2023-09-11T05:06:54Z

Pull Request Test Coverage Report forBuild 6141985002

172of181(95.03%)changed or added relevant lines in8files are covered.
12unchanged lines in5files lost coverage.
Overall coverage decreased (-0.1%) to89.759%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/spatial/bracketing.jl	35	38	92.11%
src/spatial/rssacrdirect.jl	105	111	94.59%

Files with Coverage Reduction	New Missed Lines	%
src/aggregators/prioritytable.jl	1	86.86%
src/extended_jump_array.jl	1	80.52%
src/coupling.jl	2	78.18%
src/spatial/spatial_massaction_jump.jl	3	85.45%
src/spatial/directcrdirect.jl	5	87.65%

Totals
Change from baseBuild 6133295964:	-0.1%
Covered Lines:	2419
Relevant Lines:	2695

💛 -Coveralls

Conflicts: src/spatial/bracketing.jl src/spatial/reaction_rates.jl

src/spatial/hop_rates.jl

Vilin97 · 2023-09-12T01:34:26Z

src/spatial/directcrdirect.jl

There was a very hard-to-find bug in the ordering of arguments in generate_jumps!. I fixed it here and in NSM.jl.

coveralls · 2023-09-12T04:23:20Z

Pull Request Test Coverage Report forBuild 6180696485

155of161(96.27%)changed or added relevant lines in8files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.3%) to90.175%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/spatial/bracketing.jl	23	24	95.83%
src/spatial/directcrrssa.jl	106	111	95.5%

Totals
Change from baseBuild 6167199515:	0.3%
Covered Lines:	2423
Relevant Lines:	2687

💛 -Coveralls

…to RSSACRDirect

isaacsas

This needs rebasing too it appears.

isaacsas · 2023-12-30T00:39:10Z

src/spatial/bracketing.jl

+function LowHigh(low::T, high::T; do_copy = true) where {T}
+if do_copy
+return new{T}(deepcopy(low), deepcopy(high))
+else
+return new{T}(low, high)
+end
+end
+LowHigh(pair::Tuple{T,T}; kwargs...) where {T} = LowHigh(pair[1], pair[2]; kwargs...)
+LowHigh(low_and_high::T; kwargs...) where {T} = LowHigh(low_and_high, low_and_high; kwargs...)


Does switching to using a branch have performance implications? We create these structures a lot right in the scalar case right? I'm not sure if Julia will remove it during compilation.

Even if it's not optimized away, it shouldn't be expensive, right? I am not good at reading low level code but it seems that it does get optimized away in the end.

#Old struct without branching structLowHighOld{T} low::T high::T functionLowHighOld(low::T,high::T)where{T} new{T}(deepcopy(low),deepcopy(high)) end end #New struct with branching structLowHighNew{T} low::T high::T functionLowHighNew(low::T,high::T;do_copy=true)where{T} ifdo_copy returnnew{T}(deepcopy(low),deepcopy(high)) else returnnew{T}(low, high) end end end #Test values low_value=10 high_value=20 #Code introspection to see if the branch is removed in scalar cases @code_llvmdebuginfo=:noneLowHighOld(low_value, high_value) @code_llvmdebuginfo=:noneLowHighNew(low_value, high_value; do_copy=true)

The output of the code introspection is as follows:

definevoid@julia_LowHighOld_2364([2xi64]*noaliasnocapturenoundefnonnullsret([2xi64]) align8dereferenceable(16)%0,{}* noundefnonnullreadonly%1,i64signext%2,i64signext%3) #0{ top: %newstruct.sroa.0.0..sroa_idx=getelementptrinbounds[2xi64], [2xi64]*%0,i640,i640 storei64%2,i64*%newstruct.sroa.0.0..sroa_idx,align8 %newstruct.sroa.2.0..sroa_idx1=getelementptrinbounds[2xi64], [2xi64]*%0,i640,i641storei64%3,i64*%newstruct.sroa.2.0..sroa_idx1,align8 retvoid } definevoid@julia_LowHighNew_2366([2xi64]*noaliasnocapturenoundefnonnullsret([2xi64]) align8dereferenceable(16)%0,[1xi8]*nocapturenoundefnonnullreadonlyalign1dereferenceable(1)%1,{}* noundefnonnullreadonly%2,i64signext%3,i64signext%4) #0{ top: %.sroa.025.0..sroa_idx=getelementptrinbounds[2xi64], [2xi64]*%0,i640,i640 storei64%3,i64*%.sroa.025.0..sroa_idx,align8 %.sroa.2.0..sroa_idx26=getelementptrinbounds[2xi64], [2xi64]*%0,i640,i641 storei64%4,i64*%.sroa.2.0..sroa_idx26,align8 retvoid }

src/spatial/hop_rates.jl

isaacsas · 2024-08-07T15:25:59Z

test/spatial/ABC.jl

+
+# SSAs
+for alg in [DirectCRDirect(), DirectCRRSSA()]
+push!(jump_problems, JumpProblem(prob, alg, majumps, hopping_constants = hopping_constants, spatial_system = grids[1], save_positions = (false, false), rng = rng))


Suggested change

push!(jump_problems,JumpProblem(prob, alg, majumps,hopping_constants=hopping_constants,spatial_system=grids[1], save_positions=(false,false),rng=rng))

push!(jump_problems,JumpProblem(prob, alg, majumps;hopping_constants, spatial_system=grids[1], save_positions=(false,false), rng))

src/spatial/directcrrssa.jl

isaacsas · 2024-08-07T15:41:22Z

src/spatial/directcrrssa.jl

+jump = sample_jump_direct(rx_rates.high, hop_rates.high, site, spatial_system, rng)
+time_delta += randexp(rng)
+if accept_jump(p, u, jump)
+p.next_jump_time = t + time_delta / groupsum(rt)
+p.next_jump = jump
+break
+end


Does this handle if there is no next jump?

To be honest, I do not rembmer any of the logic by now. Do we have a test for what you are asking? If not, we should, for all SSAs.

isaacsas · 2024-08-07T15:42:27Z

src/spatial/directcrrssa.jl

+if hop_rate(hop_rates.low, species, site) > acceptance_threshold
+return true
+else
+# compute the real rate. Could have used hop_rates.high as well.


Does this mean we are storing two copies of the hopping constants for the low vs high (or is it aliased)?

Aliased becausedo_copy = falsewhen initializingLowHigh(hopping_rates).Same withrx_rates.

isaacsas · 2024-08-07T15:44:01Z

src/spatial/directcrrssa.jl

+if rx_rate(rx_rates.low, rx, site) > acceptance_threshold
+return true
+else
+# compute the real rate. Could have used rx_rates.high as well.


Similar question to above

Same answer -- aliased.

src/spatial/directcrrssa.jl

into RSSACRDirect

…m/SciML/JumpProcesses.jlinto RSSACRDirect

Vilin97

Addressed comments.

Vilin97 · 2024-08-21T06:44:15Z

What do I need to do to get rid of the format and the spell check CI failures?@isaacsas

isaacsas · 2024-08-22T17:25:19Z

For spell check fix the listed spelling errors I guess. Haven’t ever used it or seen it before.

For formatting use JuliaFormatter to format the repo and add to the PR.

isaacsas · 2024-08-22T19:16:15Z

Just FYI, I'm traveling for the next week, but will try to review once I am back in Boston.

Vilin97 added2commits September 10, 2023 15:00

Write RSSACR-Direct but it's incorrect. I will turn it into CR-RSSA.

7861bc7

Fix the main part of the SSA code. Time to clean up.

e36f466

Vilin97 requested a review fromisaacsas September 11, 2023 04:39

Vilin97 added10commits September 11, 2023 20:19

Rename toDirectCRRSSA.

5ce7d25

Fix a docstring.

2a8bfa7

Shorten a function.

0d96e80

Uncomment tests inABC.jl

07873e8

Delete comment.

7fce14e

Add DirectCRRSSA to the diffusion test.

74346b2

AddDirectCRRSSAtoABC.jl.

c5f93dc

Merge branch 'master'

46f0256

Conflicts: src/spatial/bracketing.jl src/spatial/reaction_rates.jl

Shortengetindex.

fd3ea1e

Remove the low bound on site rates, as it is not used.

107068f

Vilin97 commented Sep 12, 2023

View reviewed changes

src/spatial/hop_rates.jl Outdated Show resolved Hide resolved

Vilin97 added6commits September 11, 2023 21:22

Add an@inbounds.

e1ed533

AddAbstractMatrixback in.

3290d30

Remove another change to shorten the PR.

af28404

Shorten a function.

f5bdc99

Swap order of functions.

3757606

Remove typos fromABC.jl.

22e8ece

Vilin97 marked this pull request as ready for review September 12, 2023 01:29

Vilin97 changed the title ~~[WIP] CR-RSSA~~ DirectCR-RSSA Sep 12, 2023

Vilin97 commented Sep 12, 2023

View reviewed changes

Fix test.

289a9da

Merge branch 'master' ofhttps://github /SciML/JumpProcesses.jlin…

2c0d324

…to RSSACRDirect

isaacsas requested changes Aug 7, 2024

View reviewed changes

Vilin97 added2commits August 20, 2024 21:04

Write RSSACR-Direct but it's incorrect. I will turn it into CR-RSSA.

0be2fd0

Fix the main part of the SSA code. Time to clean up.

675c140

Vilin97 added20commits August 20, 2024 21:04

Rename toDirectCRRSSA.

551af91

Fix a docstring.

5d02237

Shorten a function.

1a24b08

Uncomment tests inABC.jl

4701171

Delete comment.

4a7e562

Add DirectCRRSSA to the diffusion test.

d3bda54

AddDirectCRRSSAtoABC.jl.

d146280

Shortengetindex.

ab6a9f6

Remove the low bound on site rates, as it is not used.

97a7974

Add an@inbounds.

2b3a7a7

AddAbstractMatrixback in.

7c7a039

Remove another change to shorten the PR.

92880db

Shorten a function.

681be86

Swap order of functions.

6ed6e1d

Remove typos fromABC.jl.

0ccc09a

Fix test.

355778d

Merge branch 'RSSACRDirect' ofhttps://github /SciML/JumpProcesses.jl

558e3e0

into RSSACRDirect

Merge branch 'master' into RSSACRDirect

9148dc0

Merge branches 'RSSACRDirect' and 'RSSACRDirect' ofhttps://github.co…

1039d2e

…m/SciML/JumpProcesses.jlinto RSSACRDirect

Merge branch 'master' into RSSACRDirect

2568790

Vilin97 commented Aug 21, 2024

View reviewed changes

Address comments.

88d22b7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DirectCR-RSSA #346

DirectCR-RSSA #346

Vilin97 commented Sep 11, 2023 •

edited

Loading

github-actions bot commented Sep 11, 2023

Vilin97 Sep 12, 2023

coveralls commented Sep 12, 2023 •

edited

Loading

isaacsas left a comment

isaacsas Dec 30, 2023

Vilin97 Aug 21, 2024

isaacsas Aug 7, 2024

isaacsas Aug 7, 2024

Vilin97 Aug 21, 2024

isaacsas Aug 7, 2024

Vilin97 Aug 21, 2024

isaacsas Aug 7, 2024

Vilin97 Aug 21, 2024

Vilin97 left a comment •

edited

Loading

Vilin97 commented Aug 21, 2024 •

edited

Loading

isaacsas commented Aug 22, 2024 •

edited

Loading

isaacsas commented Aug 22, 2024

	push!(jump_problems,JumpProblem(prob, alg, majumps,hopping_constants=hopping_constants,spatial_system=grids[1], save_positions=(false,false),rng=rng))
	push!(jump_problems,JumpProblem(prob, alg, majumps;hopping_constants, spatial_system=grids[1], save_positions=(false,false), rng))

DirectCR-RSSA #346

Are you sure you want to change the base?

DirectCR-RSSA #346

Conversation

Vilin97 commented Sep 11, 2023 • edited Loading

github-actions bot commented Sep 11, 2023

Pull Request Test Coverage Report forBuild 6141985002

💛 -Coveralls

Choose a reason for hiding this comment

coveralls commented Sep 12, 2023 • edited Loading

Pull Request Test Coverage Report forBuild 6180696485

💛 -Coveralls

isaacsas left a 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

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

Choose a reason for hiding this comment

Vilin97 left a comment • edited Loading

Choose a reason for hiding this comment

Vilin97 commented Aug 21, 2024 • edited Loading

isaacsas commented Aug 22, 2024 • edited Loading

isaacsas commented Aug 22, 2024

Vilin97 commented Sep 11, 2023 •

edited

Loading

coveralls commented Sep 12, 2023 •

edited

Loading

Vilin97 left a comment •

edited

Loading

Vilin97 commented Aug 21, 2024 •

edited

Loading

isaacsas commented Aug 22, 2024 •

edited

Loading