SysBioChalmers / GECKO

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

Minor enhancement in protocol.m #279

Closed Yu-sysbio closed 1 year ago

Yu-sysbio commented 1 year ago
  1. https://github.com/SysBioChalmers/GECKO/blob/ea9e2ef56673f50b764a183fbe1a6d746f64cff3/protocol.m#L72

    If one skips this line, then running Line 73 leads to the error:

    Unrecognized function or variable 'complexInfo'.

  2. https://github.com/SysBioChalmers/GECKO/blob/ea9e2ef56673f50b764a183fbe1a6d746f64cff3/protocol.m#L87

    It is nice to display "The following reactions have invalid/incomplete EC numbers, correct them in model.eccodes and rerun getECfromGEM:", but it would be better to also suggest the correct format of the EC numbers maybe with an example like "'1.1.2.4;1.1.99.-'".

    And I just noticed that if there are any invalid/incomplete EC numbers, this function will populate model.ec.eccodes with all {''}. Even for the reactions with valid EC numbers, they will also be populated with {''}. Is this correct? If the user decides to not correct the invalid/incomplete EC numbers, then the ecModel will have no EC number populated in this step. My expectation would be that this function can at least populate EC numbers for the reactions with valid info.

  3. https://github.com/SysBioChalmers/GECKO/blob/ea9e2ef56673f50b764a183fbe1a6d746f64cff3/protocol.m#L97

    Even though there is a smilesDB.tsv file, this line queries PubChem every time.

  4. Line 119 and Line 132 are identical.

  5. https://github.com/SysBioChalmers/GECKO/blob/ea9e2ef56673f50b764a183fbe1a6d746f64cff3/protocol.m#L139

    Would it be possible to directly use obj.params.f instead of f here if the user skips Line 138?

Edit @edkerk: Actions:

edkerk commented 1 year ago

And I just noticed that if there are any invalid/incomplete EC numbers, this function will populate model.ec.eccodes with all {''}. Even for the reactions with valid EC numbers, they will also be populated with {''}. Is this correct? If the user decides to not correct the invalid/incomplete EC numbers, then the ecModel will have no EC number populated in this step. My expectation would be that this function can at least populate EC numbers for the reactions with valid info.

If it finds invalid EC numbers, it currently just returns, without touching model.ec.eccodes. If it was {''}, it will remain this way. We can indeed only populate with the correct ones and report back the problematic ones.

edkerk commented 1 year ago

Even though there is a smilesDB.tsv file, this line queries PubChem every time.

It queries PubChem for the metabolites it cannot find in smilesDB.tsv. Currently, only successful queries are stored in smilesDB.tsv, an alternative would be to also store empty entries for metabolites that no match could be found for. Not a bad idea, as the user could then curate smilesDB.tsv if they wish.

edkerk commented 1 year ago
  • Line 119 and Line 132 are identical.

This is on purpose. In between those lines, ecModel.ec.kcat is modified, and to reapply those changes to the S-matrix, you'd have to rerun applyKcatConstraints. But the reason for this should indeed be better documented.

edkerk commented 1 year ago

5. Would it be possible to directly use obj.params.f instead of f here if the user skips Line 138?

It could, but it is also nice to showcase that f-factor can be calculated from real data, instead of using the value defined in the model adapter. The text needs to be updated around that function to present this as two alternative options.