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

Useful methods for <:SSA types #42

Merged
merged 8 commits into from
Mar 26, 2024
Merged

Useful methods for <:SSA types #42

merged 8 commits into from
Mar 26, 2024

Conversation

slwu89
Copy link
Collaborator

@slwu89 slwu89 commented Mar 23, 2024

Address #37 and #36, for all of the "single samplers" adds methods getindex, length, keys, and keytype (see src/sample/interface.jl), and added tests.

Additionally:

  1. Fixed the typing for abstract methods in src/sample/interface.jl
  2. Changed the MutableBinaryHeap to a MutableBinaryMinHeap in the combined next reaction sampler
  3. @adolgert I am not sure what Base.getindex should return for the DirectCall sampler (see src/sample/direct.jl), I thought the leaf nodes of the prefix tree would be the distribution rate parameter but it does not seem to be the case. Can you help me take a look at that? That's why the newly added test in test_direct.jl is commented out. I also added Base.getindex methods to some of the trees in src/prefixsearch. This also raises the general question of what it should return as in some cases we store distributions and in other cases we store sampled times.

@slwu89 slwu89 requested a review from adolgert March 23, 2024 23:10
@adolgert
Copy link
Owner

Ah, that's right. It isn't the rate. The data structure is a prefix sum tree, aka Fenwick tree. It stores partial sums at nodes, so you have to back that out to get the hazard for an Exponential distribution. I'll take a look.

@adolgert
Copy link
Owner

Hey @slwu89, I think I fixed the problem, but I don't see the test that's commented out. I pushed to the branch with the fix. It's not a Fenwick tree. It's a simple binary tree, stored as a one-based array.

@slwu89
Copy link
Collaborator Author

slwu89 commented Mar 25, 2024

thanks @adolgert! I'll update from my end and then ping you when I think we're ready for the final review before merge.

@slwu89
Copy link
Collaborator Author

slwu89 commented Mar 25, 2024

Ok @adolgert this is ready for your review.

@adolgert adolgert merged commit d78cf8d into main Mar 26, 2024
7 checks passed
@slwu89 slwu89 deleted the sampler-interface branch March 26, 2024 14:32
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.

2 participants