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

Complex balancing #822

Merged
merged 36 commits into from
Jun 6, 2024
Merged

Complex balancing #822

merged 36 commits into from
Jun 6, 2024

Conversation

vyudu
Copy link
Collaborator

@vyudu vyudu commented May 12, 2024

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

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).

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.

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.

Project.toml Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
Project.toml Show resolved Hide resolved
docs/src/catalyst_applications/chemical_master_equation.md Outdated Show resolved Hide resolved
src/Project.toml Outdated Show resolved Hide resolved
test/network_analysis/network_properties.jl Outdated Show resolved Hide resolved
test/network_analysis/network_properties.jl Outdated Show resolved Hide resolved
src/networkapi.jl Outdated Show resolved Hide resolved
src/networkapi.jl Outdated Show resolved Hide resolved
src/networkapi.jl Outdated Show resolved Hide resolved
@isaacsas
Copy link
Member

@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.

@TorkelE
Copy link
Member

TorkelE commented May 15, 2024

I second that GitKraken is a really nice tool, if you haven't checked it out yet.

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.

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.

src/networkapi.jl Outdated Show resolved Hide resolved
src/networkapi.jl Outdated Show resolved Hide resolved
src/networkapi.jl Outdated Show resolved Hide resolved
src/networkapi.jl Outdated Show resolved Hide resolved
src/networkapi.jl Outdated Show resolved Hide resolved
src/networkapi.jl Outdated Show resolved Hide resolved
src/networkapi.jl Outdated Show resolved Hide resolved
src/networkapi.jl Outdated Show resolved Hide resolved
src/networkapi.jl Outdated Show resolved Hide resolved
src/networkapi.jl Outdated Show resolved Hide resolved
src/networkapi.jl Outdated Show resolved Hide resolved
@vyudu vyudu marked this pull request as ready for review May 23, 2024 18:47
@TorkelE TorkelE closed this May 31, 2024
@TorkelE TorkelE reopened this May 31, 2024
@isaacsas
Copy link
Member

isaacsas commented Jun 4, 2024

CI is still failing?

@isaacsas
Copy link
Member

isaacsas commented Jun 4, 2024

This looks like it might have been rebased on an older version of master?

@isaacsas
Copy link
Member

isaacsas commented Jun 4, 2024

Maybe try merging master into your PR?

@isaacsas
Copy link
Member

isaacsas commented Jun 4, 2024

(Generally I find it easier to merge instead of rebase to update a PR.)

@TorkelE
Copy link
Member

TorkelE commented Jun 4, 2024

@vyudu I tried to merge this onto master for you, but couldn't get it to work unfortunately. If you check the "Files changed" tab at the top of the PR you see that there are a very large number of changes:
image

What probably have happened is that this was a PR of some branch, and then it got rebased on an updated master, and somewhere some of the changes of the branches it originally was branched off of was carried through when this one was rebased on the master again.

Since you are developing on your own master branch, I would try (as Sam pointed out here, and to me earlier) to simply merge your master branch with Catalyst's master branch, and then push the PR again. If that does not work out, and the changes introduced here are very straightforward (i.e. just adds functions at the bottom of the network_analysis.jl file, and some tests) it might be easiest to simply copy these into a new, clean, branch and make a new PR from that.

src/Catalyst.jl Outdated Show resolved Hide resolved
@isaacsas isaacsas merged commit 5277a2c into SciML:master Jun 6, 2024
4 of 7 checks passed
@isaacsas
Copy link
Member

isaacsas commented Jun 6, 2024

Thanks @vyudu great first PR.

@TorkelE
Copy link
Member

TorkelE commented Jun 6, 2024

Thanks @vyudu great first PR.

Second that! And sorry for giving you poor advice on the rebase/merge thing. I tried @isaacsas's method properly now as well and it works great

@vyudu
Copy link
Collaborator Author

vyudu commented Jun 6, 2024

Thank you both!

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.

None yet

3 participants