Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: prepHumanModelForftINIT with paths #422

Merged
merged 5 commits into from
Nov 6, 2022

Conversation

johan-gson
Copy link
Collaborator

@johan-gson johan-gson commented Oct 12, 2022

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:

  • Tested my code on my own computer for running the model
  • Selected develop as a target branch

…he tasks and reactions.tsv. This is good when using the code for for example Mouse-GEM.
@johan-gson
Copy link
Collaborator Author

@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.

@mihai-sysbio mihai-sysbio changed the title fix: prepHumanModelForftINIT now allows for specifying the paths to t… fix: prepHumanModelForftINIT for other models Oct 20, 2022
@mihai-sysbio mihai-sysbio changed the title fix: prepHumanModelForftINIT for other models feat: prepHumanModelForftINIT with paths Oct 20, 2022
Copy link
Member

@mihai-sysbio mihai-sysbio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice improvement with explicit paths!

In this case, I find providing default values for the function more a maintenance cost than a benefit for the user. These functions are not meant to be typed that many times in an analysis, so being explicit about what files are used is a small overhead for a user but less code for maintainers to worry about. For example, if the name or path of the metabolic tasks file changes, as a maintainer, one has to remember to find (at least) this occurrence and update it as well to keep the code functional. I've made some inline suggestions accordingly.

code/tINIT/prepHumanModelForftINIT.m Show resolved Hide resolved
code/tINIT/prepHumanModelForftINIT.m Outdated Show resolved Hide resolved
code/tINIT/prepHumanModelForftINIT.m Outdated Show resolved Hide resolved
code/tINIT/prepHumanModelForftINIT.m Outdated Show resolved Hide resolved
@johan-gson
Copy link
Collaborator Author

johan-gson commented Oct 21, 2022

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.

@mihai-sysbio
Copy link
Member

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.

@johan-gson
Copy link
Collaborator Author

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.

@mihai-sysbio
Copy link
Member

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?

@johan-gson
Copy link
Collaborator Author

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.

@mihai-sysbio mihai-sysbio self-requested a review November 2, 2022 06:42
Copy link
Member

@mihai-sysbio mihai-sysbio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johan-gson please merge when you think is the right time. And a reminder - the Human-GEM-guide should probably be updated when this makes it in a release.

@johan-gson johan-gson merged commit b500274 into develop Nov 6, 2022
@haowang-bioinfo haowang-bioinfo deleted the fix/ftINITPathsAsParams branch December 1, 2022 14:23
@haowang-bioinfo haowang-bioinfo mentioned this pull request Dec 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants