Human-GEM
Human-GEM copied to clipboard
feat: prepHumanModelForftINIT with paths
This PR fixes issue #401 and now allows for specifying the paths to the tasks and reactions.tsv. This is good when using the code for for example Mouse-GEM.
Main improvements in this PR:
prepHumanModelForftINIT supports other models such as Mouse-GEM - the paths to relevant files could not be specified earlier.
I hereby confirm that I have:
- [X] Tested my code on my own computer for running the model
- [X] Selected
developas a target branch
@haowang-bioinfo @mihai-sysbio Could someone of you review this so I can merge it? It is a small change that makes it possible to run ftINIT properly for Mouse-GEM and similar GEMs.
Hi Mihail! Although you have a point, I request that we keep it as it is :) The main reason is that we otherwise break the interface, which for example will stop the code we just submitted for the single-cell modeling paper from working. I also have the argument that I don't think all users will know what a task list is, where to find it etc., and that having to supply these params makes using the function unnecessarily difficult - the first time I used tINIT I didn't know what the task file was and where to find it. The whole code around ftINIT in Human-GEM has that only purpose, to make it easier to use ftINIT - the code is not really needed in that sense, it is just a fairly simple wrapper that removes some normally unwanted reactions and fills in some standard values, such as the path to the task file. The reason for this fix is that I didn't think of the case with the mouse model, and that is why these params are needed.
The main reason is that we otherwise break the interface, which for example will stop the code we just submitted for the single-cell modeling paper from working.
This PR is to the develop branch, which I hope was not referenced in the submission. So it would only become problematic if it gets to main. Another option would be to keep this PR on hold for a few months.
I also have the argument that I don't think all users will know what a task list is, where to find it etc., and that having to supply these params makes using the function unnecessarily difficult - the first time I used tINIT I didn't know what the task file was and where to find it.
This is an important concern. My approach would be to solve this with improved documentation, by continuing to maintain the Human-GEM-guide.
The whole code around ftINIT in Human-GEM has that only purpose, to make it easier to use ftINIT - the code is not really needed in that sense, it is just a fairly simple wrapper that removes some normally unwanted reactions and fills in some standard values, such as the path to the task file.
We are on the same page here. My approach is not to simplify things towards a magical one command that does everything, but to be very clear about what each command will do.
The reason for this fix is that I didn't think of the case with the mouse model, and that is why these params are needed.
Now that this branch exists, it can be used even without merging.
I think you have a point, when I think of it the main concern would be that there is a substantial risk that people now will use the mouse model with another reactions.tsv.
For the Mouse model, when using the Mouse-GEM reactions.tsv, should people be running prepHumanModelForftINIT on Human-GEM or on Mouse-GEM?
And should consider adding an ftINIT page for how to use it with other models, say Mouse-GEM?
So, they should use it with Mouse-GEM, and with that reactions.tsv, although it probably doesn't matter. But using the right model is important.