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

Merge Pairs Of Irreversible Transport Reactions #625

Merged
merged 6 commits into from
Jun 19, 2023
Merged

Merge Pairs Of Irreversible Transport Reactions #625

merged 6 commits into from
Jun 19, 2023

Conversation

Devlin-Moyer
Copy link
Collaborator

@Devlin-Moyer Devlin-Moyer commented Jun 8, 2023

Main improvements in this PR:

For each pair of reactions mentioned in #562:

  • Makes the reaction with the lower/smaller ID number reversible
  • Copies any E.C. codes and references from the reaction with the lower/smaller ID number to the reaction with the higher/larger ID number
  • Removes the reaction with the higher/larger ID number

I hereby confirm that I have:

  • Tested my code on my own computer for running the model
  • Selected develop as a target branch
  • Any removed reactions and metabolites have been moved to the corresponding deprecated identifier lists

@feiranl
Copy link
Collaborator

feiranl commented Jun 9, 2023

Nice! but it would be great if you can also merge the cross reference annotations and rxnNotes in the model yaml file and reactions.tsv file for the further usage. I am working on the tissue specific tasks. Since recently we removed many rxns, it is difficult to find the kept rxn from the old documented rxn ID of the duplicate pair without manually search in the issue.

@Devlin-Moyer
Copy link
Collaborator Author

I will start working on that

@Devlin-Moyer
Copy link
Collaborator Author

FYI none of the removed reactions had any rxnNotes, but several had EC codes that the kept reactions didn't have, so I copied over both the references and the EC codes

@Devlin-Moyer Devlin-Moyer mentioned this pull request Jun 9, 2023
3 tasks
@haowang-bioinfo
Copy link
Member

@Devlin-Moyer good work

just a little concern about the merging: what if it turns out that some import and export reactions of the same metabolites are later associated with different transporters?

@Devlin-Moyer
Copy link
Collaborator Author

At the moment, we have no particular reason to believe that's the case (as far as I know -- correct me if I'm wrong), and the presence of these paired irreversible opposite reactions creates a bunch of loops of unbounded fluxes in the model, so it seems reasonable to merge them all now and only separate them back out again when we have a specific reason to intentionally introduce a loop into the model

@haowang-bioinfo
Copy link
Member

haowang-bioinfo commented Jun 11, 2023

the presence of these paired irreversible opposite reactions creates a bunch of loops of unbounded fluxes in the model

could you please make a list of these cases

@Devlin-Moyer
Copy link
Collaborator Author

Devlin-Moyer commented Jun 12, 2023

Every single pair of reactions in #562 forms a loop that can sustain arbitrarily large flux, since, in each pair of reactions, both have identical metabolites, and one has exactly the opposite stoichiometry as the other. I first came across these reactions by getting representative samples of solutions to Human-GEM-derived models and looking for reactions that had average possible fluxes that were suspiciously large (>100 times larger than any bound I had set on an exchange reaction)

@Devlin-Moyer
Copy link
Collaborator Author

Since there are only two reactions involved in each, they're very self-contained, but I've been spending a lot of time sampling from solution spaces of models, and each reaction with an unbounded flux is a dimension of the solution space that extends infinitely without ever hitting a bound, which makes the whole process slightly more annoying than it needs to be

@haowang-bioinfo
Copy link
Member

haowang-bioinfo commented Jun 12, 2023

what if it turns out that some import and export reactions of the same metabolites are later associated with different transporters?

At the moment, we have no particular reason to believe that's the case

ture, there is no evidence

Every single pair of reactions in #562 forms a loop that can sustain arbitrarily large flux, since, in each pair of reactions, both have identical metabolites, and one has exactly the opposite stoichiometry as the other.

yes indeed

each reaction with an unbounded flux is a dimension of the solution space that extends infinitely without ever hitting a bound, which makes the whole process slightly more annoying than it needs to be

make sense to me, what do you think @feiranl?

@feiranl
Copy link
Collaborator

feiranl commented Jun 13, 2023

I am ok with the change. As for the unbounded flux, it may be solved by using pFBA @Devlin-Moyer

@haowang-bioinfo
Copy link
Member

look good!

@haowang-bioinfo haowang-bioinfo merged commit 225e16e into SysBioChalmers:develop Jun 19, 2023
@Devlin-Moyer Devlin-Moyer deleted the fix/merge_irrev_trans_pairs branch June 20, 2023 01:21
@haowang-bioinfo haowang-bioinfo mentioned this pull request Sep 25, 2023
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