SystemsBioinformatics / ecmtool

Uncover organisms' metabolic blueprints
MIT License
12 stars 6 forks source link

ECMs without biomass, remove_infeasible_irreversible_reactions bug #10

Closed pjetroque closed 3 years ago

pjetroque commented 3 years ago

Hi,

I have a small problem with calculating the ECMs for my (relatively large) model. It seems that the function 'remove_infeasible_irreversible_reactions' mistakenly throws out reactions necessary for the ECMs with biomass. If I run the ecmtool without the 'remove_infeasible_irreversible_reactions ' step during compression it does return ECMs containing biomass (objective).

I attached my runscript and used model. data_ECMs_bug.zip

Thanks in advance, Pjotr van der Jagt University of Amsterdam

dhdegroot commented 3 years ago

Hi Pjotr, Thanks for reporting this issue. It seems that some round-off errors are affecting a nullspace computation. In this compression step we temporarily use the stoichiometric matrix with floats, while we use it in fractions for the other computations. For your model, this seems to result in very large values (~1E33). That probably gives the problem.

I will try to normalise the columns of the stoichiometric matrix, and see if that resolves the issue. I will let you know.

maxEntropyProd commented 3 years ago

I was working with ecmtool a few months back and trying different GEMs and I also found that for some GEMs, ecmtool did not generate biomass synthesis reactions, even though FBA shows biomass synthesis fluxes. For instance, the GEM for Synechocystis found here: http://bigg.ucsd.edu/models/iJN678 does not generate ECMs associated with biomass synthesis. I had to move on to some other projects before figuring out the reason, but I plan to return to exploring ecmtool in the near future.

I'm not sure if it's related to the above problem, but it sounds similar. Ecmtool is exactly what I need, and you guys did some great working developing it!

cheers, -joe

dhdegroot commented 3 years ago

@pjetroque, I think the issue has been resolved. I have created a pull request, and @tjclement will review that shortly. After that, I'll merge the updated version and you're good to go.

dhdegroot commented 3 years ago

@maxEntropyProd, We are happy to hear that ecmtool is potentially useful for you, and together we can probably fix your issue. I'd ask you to check the following: 1) is the issue solved by the updated version? (you can of course only check that when it has been merged later today)

If not, most of the problems that we encounter when ecmtool is used with different models, are model parsing issues. This is because there are different conventions of storing a model, and we can't accommodate all of them. Therefore, we introduced the arguments --print_metabolites True, and --print_reactions True. These can be given when ecmtool is run from the command line (as we recommend). Things you can then check:

I tried running the iJN678-model, but ecmtool gave an error message that there was a reaction ("R_EX_photon_e") with both lower and upper bound equal to 0. We chose to not remove these reactions ourselves, but leave this to the user, so as not to introduce many bugs. Do you have a model where such reactions are removed?

Best, Daan

dhdegroot commented 3 years ago

Hi Pjotr (@pjetroque) In the latest version your issue should be resolved. @maxEntropyProd , I will close this issue as solved. If your issue pertains, please open a new issue, and we will help you further.

Best, Daan

maxEntropyProd commented 3 years ago

Great, thanks so much! I had followed the suggestions in your manual/paper regarding checking for metabolites, reversibility, etc, and I had removed bounds or reactions that were flagged by ecmtool.

Hopefully I will be able to get back to the project in the next week or two and will try the latest patch. Thanks again for all of your feedback and code fixes!

cheers, -joe