-
Notifications
You must be signed in to change notification settings - Fork 41
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
Fix: Make all non-transport/exchange pyrophosphate reactions irreversible #557
Conversation
Oh dang; I used Cobrapy's built-in function for saving models in YAML format, but it appears to have not used the format your automated format checker was expecting |
looks good I'm afraid the effect of these changes needs to be checked with verification and essentiality tests that are under construction, will come back as soon as possible |
actually is considering to fix the compatibility to cobrapy, any ideas in improving this? |
Cobrapy uses ruamel.yaml to generate their YAML files. I'm not certain if these are all of the differences, but:
I suspect that the biggest issues are the contents of the header and the quotes around metabolite formulas; I think fixing the header problem would require changes on Cobrapy's end, because, as far as I can tell, most of the information you have in the header of your YAML file isn't in a Cobrapy YAML file at all. The other things seem like things you could probably tweak with your YAML validation thing to accommodate, but I'm not entirely sure how it works. Also it may be worth noting that Cobrapy had no problem reading in Human-GEM.yml, as far as I could tell |
While I realise it is more cumbersome, SBML is a more robust file format for using the model across different tools/platforms. |
yes, the reading was fixed early on; now probably is time to resolve writing side
thanks for listing the details
the quote issue is technically solvable, while header section actually is the tricky part |
yes SBML is a mess |
This might be the better solution in more than one case. |
@Devlin-Moyer would you be able to merge the latest |
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.
very nice - significant progress in improving thermodynamic constrains!
Main improvements in this PR:
Makes all reversible non-transport/exchange reactions that involve pyrophosphate (MAM02759e/c/m/x/n/r) irreversible in the direction of pyrophosphate production, as discussed in #527. In cases where pyrophosphate is currently a “reactant,” products and reactants are swapped to ensure no reactions are constrained to only have fluxes <= 0.
I hereby confirm that I have:
develop
as a target branch