search

AlexsLemonade / OpenPBTA-analysis

The analysis repository for the Open Pediatric Brain Tumor Atlas Project
Other
99 stars 66 forks source link

Update oncoprint panel figure #1164

Closed cbethell closed 3 years ago

cbethell commented 3 years ago

Purpose/implementation Section

This PR closes #1148

It updates figures/scripts/oncoprint-landscape.R to include 4 instead of 5 panels because, as noted in #1148, #1143 combined ependymal tumors into other.

What scientific question is your analysis addressing?

No specific question is being addressed. Updates are being made to the oncoprint landscape figure generation script as to propagate changes from the updated analysis in #1143.

What was your approach?

Pulled the latest docker image and updated the local master of this repo before modifying figures/scripts/oncoprint-landscape.R to include only 4 panels in each of the output files (namely, figures/pngs/primary-plus_oncoprint_landscape.png and figures/pngs/primary_only_oncoprint_landscape.png), removing the ependymal tumor panel.

What GitHub issue does your pull request address?

1148

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

Which areas should receive a particularly close look?

Do the final figures look reasonable? Are there formatting changes we would like to make?

Is there anything that you want to discuss further?

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)?

The following plots have been updated:

What is your summary of the results?

The 5th panel appears to have successfully been removed and the plots incorporated appear to be the latest as updated in master.

Reproducibility Checklist

Documentation Checklist

jharenza commented 3 years ago

Thanks for submitting this @cbethell !

Are there formatting changes we would like to make?

I currently have this ticket in for some changes and @kgaonkar6 was able to do all of them except for Increase size of the CNV boxes? Make a rectangle? which I realize we would have to change in the maftools package, so I am not sure how easy it is to do that on our timeline.

In looking at the panel figure, I could also suggest some other changes, but for aesthetics, but this can probably be another ticket:

Let me know your thoughts as to what should be (if any) tackled here, and in the meantime, I will create a new ticket.

jharenza commented 3 years ago

created ticket https://github.com/AlexsLemonade/OpenPBTA-analysis/issues/1165

cbethell commented 3 years ago

In looking at the panel figure, I could also suggest some other changes, but for aesthetics, but this can probably be another ticket:

  • remove titles on each plot (Altered in*)
  • Move broad histologies titles to be centered above each plot
  • Label A,B (top), C,D (bottom)
  • Increase gene name font size as much as possible
  • Try to make the plots more horizontally condensed for a portrait page
  • Update germline_sex_estimate label to sex and cancer_group to histology(?) and make sure they are not cut off in the figures
  • Create a unified legend and add to the right side of the plots (I feel like this is a ticket in itself and maybe @kgaonkar6 can tackle - we would maybe have to make color boxes with labels and make them another figure, then put it into this panel - thoughts? (Since there are a lot of samples which are not displayed, some cancer groups in the legend do not have these samples in the plots, so I think we would want to only list those which have samples). It would also be nice to convert these so there are no underscores and the whole words are displayed (eg - In_Frame_Ins --> In-frame insertion) - honestly, I just made these in excel before (ick) and created a piece of a figure and put it in the final figure, so if this is a huge coding challenge, it can be done like that, as tedious as it is.

Let me know your thoughts as to what should be (if any) tackled here, and in the meantime, I will create a new ticket.

Hi @jharenza, thank you for filing that issue! After taking another look at the code, I believe the first six bullet points mentioned above can be addressed here! Namely:

  • remove titles on each plot (Altered in*)
  • Move broad histologies titles to be centered above each plot
  • Label A,B (top), C,D (bottom)
  • Increase gene name font size as much as possible
  • Try to make the plots more horizontally condensed for a portrait page
  • Update germline_sex_estimate label to sex and cancer_group to histology(?) and make sure they are not cut off in the figures

As far as addressing your point re: Increase size of the CNV boxes? Make a rectangle? -- I am happy to take a look into this but this may or may not be outside the scope of this PR (if it is I will comment on the relevant ticket the probable next steps that will need to be made to address it).

jharenza commented 3 years ago

thanks @cbethell - sounds great!

cbethell commented 3 years ago

In one of the most recent commits, I believe I was able to successfully implement the following changes:

Many of the changes made were only possible when installing the maftools package from github so I also modified the package installation in the Dockerfile -- will request @jaclyn-taroni's review here mainly for this purpose.

jharenza commented 3 years ago

Thanks so much for the updates @cbethell!

Try to make the plots more horizontally condensed for a portrait page -- this may possibly further be improved with the removal of the whitespace on either side of the plots, is this something that seems closer to what we may want here @jharenza?

Yes, if possible!

cbethell commented 3 years ago

Try to make the plots more horizontally condensed for a portrait page -- this may possibly further be improved with the removal of the whitespace on either side of the plots, is this something that seems closer to what we may want here @jharenza?

Yes, if possible!

Done! 👍

cbethell commented 3 years ago

Hi @cbethell! thanks for all of the great updates here! I think the below one was only partially addressed, but happy to have this come later.

Update germline_sex_estimate label to sex and cancer_group to histology(?) and make sure they are not cut off in the figures

For this, I meant for us to remove all of the underscores and have some shorter terminology (sex), although cancer group without the underscore is also fine if we want to just keep it consistent with the column name. So, I will approve (these are minor things we can fix whenever) since the major updates are in.

Ah yes @jharenza, when I dove into attempting to update this, it was not as straightforward to address as I thought so I figured that would best be included in a later PR with the remaining items on #1165!

jharenza commented 3 years ago

Sounds good!