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

fix: constructEquations #202

Merged
merged 6 commits into from
Mar 29, 2019
Merged

fix: constructEquations #202

merged 6 commits into from
Mar 29, 2019

Conversation

haowang-bioinfo
Copy link
Member

@haowang-bioinfo haowang-bioinfo commented Dec 14, 2018

Main improvements in this PR:

  • fix: get the reversibility information by querying both lb and ub fields, instead of the obsolete rev field that could be inaccurate or unavailable
  • doc: refine function description with Input and Output sub-headings
  • feat: additional option is added for allowing the generation of equations with met formulas

NOTE: None of these changes affect backward-compatibility of any of the modified functions. All reactions in a model should be organized in their forward direction, typically by setting ub = 1000 and lb = -1000 (or 0), so that their equations can be correctly constructed by the constructEquations function.

I hereby confirm that I have:

- Previously the reversibility is obtained from the `rev` field, which could be inaccurate and even unavailable sometimes. This fixation uses `lb` field to get the reversibility and avoids the obsolete `rev` field.
@edkerk
Copy link
Member

edkerk commented Dec 14, 2018

It makes a lot of sense to stop using the rev field, but it needs to be mentioned explicitely in the documentation that the lb fields is queried for determining reversibility. There can be cases where ub = 0 and lb = -1000, is this strictly reversible? True, the reaction should then have been defined in the other direction, but it can be good to mention this.

Something like:

All reactions are defined in their forward direction, while the lb field is queried to determine whether reactions can also support a flux in reverse direction.

@edkerk
Copy link
Member

edkerk commented Dec 20, 2018

Also, in the process of abandoning the use of the rev field, the functionality of convertToIrrev should be changed, as this currently uses the rev field to identify what reactions are reversible.

@BenjaSanchez
Copy link
Contributor

@edkerk based on a previous discussion in #184 (comment), will the rev vector still be able to be read by convertToIrrev with some readRevVector=TRUE flag? As GECKO uses convertToIrrev as one of the first steps, would be nice to know so I can handle the change later.

In this commit, the reversibility is obtained by querying both `lb` and `ub` fields. This is to avoid the obsolete `rev` field and weird cases (e.g. ub=0 and lb=-1000). Note that all reactions in a model should be defined in their forward direction, so that their equations could be properly formulated by this function.
@haowang-bioinfo
Copy link
Member Author

haowang-bioinfo commented Feb 25, 2019

How about having an option of constructing reactions using metFormulas, this appears to be quite useful when balancing reaction.

- So far, reaction equations can be generated with either met names or met ids. Here an additional option is added for allowing the generation of equations with met formulas. This feature is found to be useful when checking the mass balance of reactions.
@edkerk
Copy link
Member

edkerk commented Mar 5, 2019

How about having an option of constructing reactions using metFormulas, this appears to be quite useful when balancing reaction.

Sure! I'm not sure when this is useful, but if you find a use for it that's great. You've implemented it correctly, not breaking previous functionality, so I have no problem with this part.

@haowang-bioinfo haowang-bioinfo merged commit 9a88881 into devel Mar 29, 2019
@haowang-bioinfo haowang-bioinfo deleted the fix/constructEquations branch March 29, 2019 10:06
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.

3 participants