SysBioChalmers / GECKO

Toolbox for including enzyme constraints on a genome-scale model.
http://sysbiochalmers.github.io/GECKO/
MIT License
64 stars 46 forks source link

feat: Add new rxns to ecModel & fix: ecFSEOF #337

Closed ae-tafur closed 1 year ago

ae-tafur commented 1 year ago

Main improvements in this PR:

I hereby confirm that I have:

github-actions[bot] commented 1 year ago

This PR has been automatically tested with GH Actions. Here is the output of the tests:

 
Running geckoCoreFunctionTests
Done geckoCoreFunctionTests
__________

Note: In the case of multiple test runs, this post will be edited.

edkerk commented 1 year ago

Looks good, will test it out. A few things:

ae-tafur commented 1 year ago
  • Probably good to mention about setKcatForReactions and applyKcatConstraints in the function description.
  • The old function name (getEngineeredModel) is occassionally mentioned.

True, I will fix that.

  • Should newEnzymes not be an optional field? The gene/enzyme might already be present. Together with that, there should be a check whether the gene/enzyme not already exist (ok, addGenesRaven does that check, but its warning message will seem strange as the user will not have defined a genesToAdd structure themselves, nor does it handle enzymes).

Do you mean ´new enzymes´ as a field of ´newRxns´? I was thinking it as individual input because the extra info we need (mw, and enzyme id). Maybe this way is easier for the user ?

Validation if gene/enzyme might already be present was added

edkerk commented 1 year ago

Sorry, I meant to have newEnzymes is an optional input for running addNewRxnsToEC.m. If I know that an enzyme is present, do I really have to provide newEnzymes information.

I now see that you indeed have a check that errors if one of the enzymes is present. Another way to deal with that is to check if the enzyme is present, if present remove it from the list of newEnzymes, and then only run steps 2, 4, 5 and partial step 6 for any truly new enzymes. But if I now indeed add a reaction with only existing enzymes, I will get an error that these should be removed from newEnzymes, after which I will have to provide an empty newEnzymes, which will then error at these steps when pseudometabolite is added etc.

ae-tafur commented 1 year ago

Sorry, I meant to have newEnzymes is an optional input for running addNewRxnsToEC.m. If I know that an enzyme is present, do I really have to provide newEnzymes information.

I now see that you indeed have a check that errors if one of the enzymes is present. Another way to deal with that is to check if the enzyme is present, if present remove it from the list of newEnzymes, and then only run steps 2, 4, 5 and partial step 6 for any truly new enzymes. But if I now indeed add a reaction with only existing enzymes, I will get an error that these should be removed from newEnzymes, after which I will have to provide an empty newEnzymes, which will then error at these steps when pseudometabolite is added etc.

OK, Initially, my idea was to add reactions that are not part of the model because new genes are incorporated to the organism. So, I was assuming that if a gene is already part of the genome of that model, the reaction should be there. But I think I got you point.

Now is adapted to do that.