Skip to content
This repository has been archived by the owner on Jun 21, 2023. It is now read-only.

Update bgCol in oncoprint module #975

Merged
merged 4 commits into from
Apr 1, 2021

Conversation

cbethell
Copy link
Contributor

Purpose/implementation Section

The purpose of this PR is to update the bgCol of the oncoprint figures.

What scientific question is your analysis addressing?

This is an update to an existing analysis.

What was your approach?

As recommended in #966 (comment), this PR updates the oncoplot() function in oncoprint-landscape/01-plot-oncoprint.R to use bgCol = "#F5F5F5" and re-runs the module script to update the figures in analyses/oncoprint-landscape/plots.

I also re-ran the oncoprint section of the figures/generate-figures.sh script to update the pub-ready oncoprint.

What GitHub issue does your pull request address?

This PR addresses #966

Directions for reviewers. Tell potential reviewers what kind of feedback you are soliciting.

Which areas should receive a particularly close look?

Does the background color of the updated plots look as expected?

Is there anything that you want to discuss further?

If I interpreted your comment here correctly @jharenza:

I am not sure why the filenames are not printing as expected - they seem to be coded correctly

It appears that the filenames are indeed printing as expected, but the plots themselves may be displayed a bit differently than one would expect.

I came to this conclusion because I cross-checked the genes included in our genes of interest list (compiled of analyses/interaction-plots/results/gene_disease_top50.tsv and analyses/focal-cn-file-preparation/results/consensus_seg_focal_cn_recurrent_genes.tsv and determined that the genes represented on the _goi_oncoprint.png plots are synonymous with those represented in this list.

With that being said, it appears that a default behavior of the oncoplot() function is to display the top 20 genes with mutations, a functionality that can be over-ridden if supplied a genes of interest list with more than 20 genes. This is what seems to be happening in our case as when I tried supplying the argument top = 50 rather than the default top = 20 and re-running the script, the goi plots remained unchanged while the plots not using goi were updated with more genes. Would you agree?

See the comparison of the plots when adjusting this argument below:

with top = 50
all_participants_primary_only_oncoprint

with the default top = 20
all_participants_primary_only_oncoprint

Question for reviewers: Do we want to update the top argument for the plots that do not use the goi or do we want to wait for the updated goi (mentioned in #969 (comment)) before making that decision?

Is the analysis in a mature enough form that the resulting figure(s) and/or table(s) are ready for review?

Yes.

Results

What types of results are included (e.g., table, figure)?

No new results, just updated the oncoprint plots.

What is your summary of the results?

No new results to summarize.

Reproducibility Checklist

  • The dependencies required to run the code in this pull request have been added to the project Dockerfile.
  • This analysis has been added to continuous integration.

Documentation Checklist

  • This analysis module has a README and it is up to date.
  • This analysis is recorded in the table in analyses/README.md and the entry is up to date.
  • The analytical code is documented and contains comments.

@cbethell cbethell requested a review from jharenza March 30, 2021 20:36
@jharenza
Copy link
Collaborator

jharenza commented Mar 31, 2021

Hi @cbethell ! Thanks for checking in on this. The bgCol looks good! Could we also change the multi-hit color while we have this open, since it is pretty bright, doesn't really fit with the other colors, and we have many multi-hits here?

I actually didn't catch the top behavior, but I think top 20 or even 25 would be fine, but 50 would likely be too much for the visual.

analyses/focal-cn-file-preparation/results/consensus_seg_focal_cn_recurrent_genes.tsv

Ahh, I think I missed that this was going into the goi list, and had thought the oncoprints without the (what seem to be artifactual) CN losses were not the goi lists and that the ones not labeled with goi were coming from the goi list because those genes are mostly biologically relevant here. We are also re-assessing CN with #964, so hopefully, this will change. It is odd that of all the recurrent CNVs, I am only seeing deletions and the fact that all deletions are occurring in the same samples is perhaps suggesting either these are artifacts or these genes are all on one chr arm which is deleted, so they would not be focal. However, this is way past beyond the scope of your PR :), and I will look closer into that analysis once #964 is completed.

That being said, if we have recurrent SNVs and CNVs in our GOI lists, then it might follow that we add recurrent fusions or fused genes, which can be found in this results folder. Since the recurrent fusion analysis was done per histology, and there are not very many, we could simply add these genes to our goi list. I can add a separate ticket for that since this is also beyond the initial ticket.

Update: I created #977 for the fusion genes of interest.

- add `top = 25` flag
- change multiHit hex code
@cbethell
Copy link
Contributor Author

Thanks for the review @jharenza!

Could we also change the multi-hit color while we have this open, since it is pretty bright, doesn't really fit with the other colors, and we have many multi-hits here?

I changed the multi-hit color in the last commit (I agree it was pretty bright), so you can let me know what you think of the updated color! I'll be happy to update it if you had something else in mind.

I actually didn't catch the top behavior, but I think top 20 or even 25 would be fine, but 50 would likely be too much for the visual.

I also updated the top argument to top = 25.

Overall, let me know what you think of the oncoprints now (as far as the scope of this PR that is)!

Copy link
Collaborator

@jharenza jharenza left a comment

Choose a reason for hiding this comment

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

looks good! thanks for the update!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants