Skip to content
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

[Ready for Review] - Constant-complexity NRM implementation #455

Merged
merged 34 commits into from
Oct 18, 2024

Conversation

vyudu
Copy link
Contributor

@vyudu vyudu commented Sep 19, 2024

An implementation of the method in Sanft/Othmer.

Copy link
Contributor

@Vilin97 Vilin97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR!

  1. Can you please explain what is the difference between this SSA and DirectCRDirect (same Sanft 2015 paper is cited there) and also DirectCR (the binning strategy looks similar)? Thank you.

  2. It would be helpful if you reported the relative performance on the new SSA on one or two problems, e.g. from tests, compared to other SSAs in the library. If it's much slower (>2x) than similar binning SSAs like DirectCR, we will need to look into it more.

  3. Also, I left some more specific comments about the code.

src/aggregators/ccnrm.jl Outdated Show resolved Hide resolved
src/aggregators/prioritytable.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
src/aggregators/ccnrm.jl Outdated Show resolved Hide resolved
@vyudu
Copy link
Contributor Author

vyudu commented Sep 20, 2024

Hi Vasily, the main difference with DirectCR is that the priority table in this method stores next reaction times rather than reaction rates, and this implementation doesn't handle spatial jumps like DirectCRDirect. But admittedly I am a little confused about the citation for DirectCRDirect being the Sanft/Othmer paper -- they do discuss a spatial generalization of CCNRM, but it does not seem equivalent to DirectCRDirect (since it involves storing reaction times instead of rates).

Performance-wise this is definitely a WIP (will convert it to a draft). Like you say it is slower and allocates much more memory than DirectCR or NRM (which is why I currently have the allocations test commented out, since it fails), but am submitting it in this state to hopefully get some feedback on how to improve this aspect, cc @isaacsas. Working on rewriting the methods for the PriorityTimeTable to allocate less.

@vyudu vyudu marked this pull request as draft September 20, 2024 00:47
src/aggregators/ccnrm.jl Outdated Show resolved Hide resolved
@isaacsas isaacsas changed the title Constnat-complexity NRM implementation [WIP] - Constant-complexity NRM implementation Sep 25, 2024
@vyudu vyudu marked this pull request as ready for review September 26, 2024 19:45
@vyudu vyudu changed the title [WIP] - Constant-complexity NRM implementation [Ready for Review] - Constant-complexity NRM implementation Sep 26, 2024
Copy link
Member

@isaacsas isaacsas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure to @inbounds hot loops with indexing when this is ready to merge.

src/aggregators/ccnrm.jl Outdated Show resolved Hide resolved
src/aggregators/ccnrm.jl Outdated Show resolved Hide resolved
src/aggregators/ccnrm.jl Outdated Show resolved Hide resolved
src/aggregators/prioritytable.jl Outdated Show resolved Hide resolved
src/aggregators/prioritytable.jl Show resolved Hide resolved
src/aggregators/prioritytable.jl Show resolved Hide resolved
src/aggregators/prioritytable.jl Outdated Show resolved Hide resolved
src/aggregators/prioritytable.jl Outdated Show resolved Hide resolved
src/aggregators/prioritytable.jl Outdated Show resolved Hide resolved
src/aggregators/prioritytable.jl Outdated Show resolved Hide resolved
@isaacsas
Copy link
Member

Please revert the format commits. They are changing lots of other stuff and there is apparently a bug in JuliaFormatter anyways that is causing formats to break semantics.

@isaacsas
Copy link
Member

@vyudu is this all done / updated now?

@vyudu
Copy link
Contributor Author

vyudu commented Oct 18, 2024

Should be after these tests run.

@isaacsas isaacsas merged commit 1f0bb70 into SciML:master Oct 18, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants