-
-
Notifications
You must be signed in to change notification settings - Fork 71
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
Complex balancing #822
base: master
Are you sure you want to change the base?
Complex balancing #822
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me start by saying this is a great first PR!
Here are a first set of comments -- these are more general Julia practices / Catalyst internals comments than method specifc or performance specific.
I need to finish going through the paper and then I will give a second review more focused on the algorithmic details, but you can at least address these for now.
@vyudu can you merge master into this? Do you know how to do that? (If not I can explain to you on Slack.) I usually use Gitkraken to do that as I find it easier than merging via the commandline. |
I second that GitKraken is a really nice tool, if you haven't checked it out yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add one more test that uses a network that is complex balanced with multiple nonlinearities and non-trivial rate expressions (i.e. reactions like k1*k2, 3A + 2B --> 3*C
)? Just to stress the code a bit more.
In terms of performance optimization I think that can be a followup. So I'm happy to merge once you address this last set of comments.
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
A first pass at the complex-balance implementation. Does not seem like Graphs.jl has a matrix-tree implementation, so I have the matrix tree implementation here for now, but I'll work on adding it as a PR to Graphs.jl (and in particular implementing a method of generating the spanning trees that doesn't simply generate every combination of n-1 edges).