-
-
Notifications
You must be signed in to change notification settings - Fork 79
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
Full system balancing #851
Conversation
src/chemistry_functionality.jl
Outdated
error("Could not balance reaction `$rx`, unable to create a balanced `ReactionSystem`.") | ||
end | ||
if length(brxs) > 1 | ||
error("Infinite number of balanced reactions possible for reaction ($rx) are possible. No method for automatically generating a valid reaction is currently implemented in `balance_system`.") |
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.
I was unsure how to actually construct a proper reaction from the base. Had a look at some outputs but didn't really understand. At some point we should support it, but not really a priority.
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.
Although we probably could raise a issue that it should be fixed
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.
We should probably update these error messages as they are imprecise (the old one too). The issue isn't an infinite number of solutions (there are always an infinite number), it is whether there are multiple independent solution vectors (i.e. the dimension of the solution space is greater than one). So the error messages should probably be updated to reflect this.
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.
Modulo that comment this is fine with me.
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.
That make sense, I will update with new error/warning message (new and old ones)
61c7572
to
fe2f492
Compare
New messages: waring: error (for system-level balancing) |
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.
If tests pass happy for you to merge this.
Creates a function
balance_system
, which takes aReactionSystem
and returns a version with all its reactions balanced.