SysBioChalmers / GECKO

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

feat: light_ecModel tutorial getSubsetEcModel #322

Closed edkerk closed 1 year ago

edkerk 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:

 
[Warning: Cannot find RAVEN Toolbox in the MATLAB path, or the version is too old (before v2.8.3). Make sure you have installed RAVEN in accordance to the following instructions,
including running 'checkInstallation()': https://github.com/SysBioChalmers/RAVEN/wiki/Installation]
[> In GECKOInstaller.checkRAVENversion (line 77)
In GECKOInstaller.install (line 13)]

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

mihai-sysbio commented 1 year ago

I'm wondering if perhaps this would also be a good opportunity to address the minor issue around:

writeDLKcatInput(ecModel);

Since there is a DLKcat.tsv in the /data folder, the code will trigger error that the file already exists. One option would be to specify all the parameters in the function, thus sending an overwrite argument. Another would be to rename the file, and write up a README in each tutorial folder that would also cover /data and why the DLKcat-backup.tsv is provided.

edkerk commented 1 year ago

With #321 merged, this PR can be reviewed.

I'm wondering if perhaps this would also be a good opportunity to address the minor issue around:

writeDLKcatInput(ecModel);

Since there is a DLKcat.tsv in the /data folder, the code will trigger error that the file already exists. One option would be to specify all the parameters in the function, thus sending an overwrite argument. Another would be to rename the file, and write up a README in each tutorial folder that would also cover /data and why the DLKcat-backup.tsv is provided.

IMHO, I don't think it's a minor issue, I'm not too happy with either of these two options. 1: Overwriting the file with an empty DLKcat.tsv (without kcat values) forces the user to run DLKcat to continue with the tutorial, which is not ideal as running DLKcat can take long, requires Docker etc. Of course the user can just restore the file, or re-download the DLKcat.tsv, but this feels sub-optimal. 2: Having a differently named file makes the tutorials sub-folder not present a "normal" situation, with alternatively named files. The user might end up with two copies (DLKcat.tsv and DLKcat-backup.tsv), which is confusing.

A third option could be to command-out the writeDLKcatInput command in the protocol.m, and explain why this is done.

ae-tafur commented 1 year ago

A third option could be to command-out the writeDLKcatInput command in the protocol.m, and explain why this is done.

I agree with the third option, but this means that runDLKcat will not work, right ? It needs the correct rxns id

edkerk commented 1 year ago

A third option could be to command-out the writeDLKcatInput command in the protocol.m, and explain why this is done.

I agree with the third option, but this means that runDLKcat will not work, right ? It needs the correct rxns id

I think you could run

A third option could be to command-out the writeDLKcatInput command in the protocol.m, and explain why this is done.

I agree with the third option, but this means that runDLKcat will not work, right ? It needs the correct rxns id

If the user wants to run runDLKcat, it will still be able to run either on the existing DLKcat.tsv file that already has kcat values (it will just overwrite those with new values, will double-check this right now), or with a new DLKcat.tsv if the user has uncommented writeDLKcatInput.

edkerk commented 1 year ago

Will re-run the whole light_ecModel tutorial and save the final model after merging other PRs, as they will also have some minor impact.