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: add gene essentiality check using DepMap #574

Merged
merged 8 commits into from
Apr 23, 2024

Conversation

johan-gson
Copy link
Collaborator

@johan-gson johan-gson commented May 21, 2023

Main improvements in this PR:

Added evaluation of gene essentiality using DepMap data. Note that the current code only runs 2 chunks out of 40 (let's change this when producing figures for the paper). Also, it currently only runs it for the model in the repo - we will have to run it in the same way for an older model version and modify the code slightly to assemble the data from both runs. Note that this PR was based on the hart2015 PR since that code was needed before it was approved.

I hereby confirm that I have:

  • Tested my code on my own computer for running the model
  • Selected develop as a target branch
  • Any removed reactions and metabolites have been moved to the corresponding deprecated identifier lists

…ion. I don't think that code had ever been run, it was just not right.
…urrently only processes the model available in the repo, and does not compare with any older model version. The code is however prepared for comparing versions. Currently, it only processes 2 out of 40 chunks of data, this is to be changed when we want to run a full evaluation with 891 cell lines.
@mihai-sysbio mihai-sysbio changed the title Feat/add gene ess dep map feat: add gene essentiality check using DepMap May 22, 2023
@haowang-bioinfo
Copy link
Member

looks good - except that it suppose to be started from develop rather than a PR in draft, better change this @johan-gson

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.

Great job! I've left some aesthetic comments.

In terms of the bigger picture, what is the aim here? To be able to run this once, now and again? Or more often, say with every other release?

code/DepMapGeneEss/ConvertGeneEssInputData.R Outdated Show resolved Hide resolved
code/DepMapGeneEss/Instructions.txt Outdated Show resolved Hide resolved
code/DepMapGeneEss/PlotGeneEss.R Outdated Show resolved Hide resolved
code/DepMapGeneEss/Instructions.txt Outdated Show resolved Hide resolved
code/DepMapGeneEss/PlotGeneEss.R Show resolved Hide resolved
code/DepMapGeneEss/PrepDepMapData.m Outdated Show resolved Hide resolved
code/DepMapGeneEss/enrichment_test.m Outdated Show resolved Hide resolved
@johan-gson
Copy link
Collaborator Author

looks good - except that it suppose to be started from develop rather than a PR in draft, better change this @johan-gson

I had to do that, I needed the code from that PR before it was approved. We did like this often in GECKO3. I'm thinking we can do like this?

@johan-gson
Copy link
Collaborator Author

Great job! I've left some aesthetic comments.

In terms of the bigger picture, what is the aim here? To be able to run this once, now and again? Or more often, say with every other release?

I'm thinking it is twofold - both at every release but also as a oneoff for generating a figure for the paper. In the second case, it takes some tweaking. This is why I left some commented out code, it will be useful for that purpose.

@haowang-bioinfo
Copy link
Member

I had to do that, I needed the code from that PR before it was approved. We did like this often in GECKO3. I'm thinking we can do like this?

this is fine - To avoid confusion, maybe clarify in PR message that the code is based on that PR?

@johan-gson
Copy link
Collaborator Author

johan-gson commented May 22, 2023

I had to do that, I needed the code from that PR before it was approved. We did like this often in GECKO3. I'm thinking we can do like this?

this is fine - To avoid confusion, maybe clarify in PR message that the code is based on that PR?

Done

@feiranl
Copy link
Collaborator

feiranl commented May 22, 2023

Great job! I've left some aesthetic comments.

In terms of the bigger picture, what is the aim here? To be able to run this once, now and again? Or more often, say with every other release?

I would say with the larger essentiality check for DepMap data should be performed every major release in the main branch. But for each PR, then we can run a smaller scale test as Hao committed in #563

@haowang-bioinfo
Copy link
Member

I would say with the larger essentiality check for DepMap data should be performed every major release in the main branch. But for each PR, then we can run a smaller scale test as Hao committed in #563

sounds a reasonable plan given the heavy computation load of DeepMap data

@mihai-sysbio
Copy link
Member

sounds a reasonable plan

Great! Perhaps this should be added to https://github.com/SysBioChalmers/Human-GEM/wiki/How-to-make-a-new-release so as not to forget about this step.

@johan-gson
Copy link
Collaborator Author

Very nice, @mihai-sysbio, I had not seen this before. It solves the problem!

@mihai-sysbio
Copy link
Member

Through the previous commit a07b7b6, the file Instructions.txt was removed. Instead, a reformatted set of instructions has been drafted for addition in the Human-GEM-guide PR SysBioChalmers/Human-GEM-guide#10.

@mihai-sysbio
Copy link
Member

Great! Perhaps this should be added to https://github.com/SysBioChalmers/Human-GEM/wiki/How-to-make-a-new-release so as not to forget about this step.

@mihai-sysbio mihai-sysbio self-requested a review November 28, 2023 09:39
@mihai-sysbio
Copy link
Member

I think this is ready for merging.

@feiranl feiranl merged commit afabd27 into develop Apr 23, 2024
@JHL-452b JHL-452b mentioned this pull request May 30, 2024
@edkerk edkerk deleted the feat/AddGeneEssDepMap branch June 24, 2024 23:08
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.

4 participants