search

AlexsLemonade / OpenPBTA-analysis

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

V22 chromothripsis figure update to use cancer group display #1457

Closed jharenza closed 2 years ago

jharenza commented 2 years ago

❗ stacked on #1401

Purpose/implementation Section

What scientific question is your analysis addressing?

Updates to use cancer_group_display for chromothripsis plot

What was your approach?

Updated code as above

What GitHub issue does your pull request address?

1368

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

Which areas should receive a particularly close look?

Just make sure I did not miss any

Is there anything that you want to discuss further?

No

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

Updated figure 3 panel

What is your summary of the results?

Reproducibility Checklist

Documentation Checklist

jharenza commented 2 years ago

In addition, one small change might be beneficial even though slightly beyond the scope here. The x-axis in the plot has angled labels but definitely doesn't need them - the labels should all fit fine at the default, so the angling just adds visual noise. Can we keep those at default? This just involves modifying line 102.

Done

jharenza commented 2 years ago

"Approved!" 🚀

@jharenza which do you think will be easier for you - should I approve now but add a don't merge label, or wait to approve until #1401? Both fine with me!

I may want to rerun after #1401 just in case we add samples to any groups, so either way works. definitely can use a dont merge regardless!

sjspielman commented 2 years ago

Ok, if there's potential for a full re-review, I won't approve yet! I realize we should definitely make #1401 a blocker instead of (or on top of) a label since that will be a little more protected.

jharenza commented 2 years ago

@sjspielman this one is also ready for re-review

sjspielman commented 2 years ago

I'm realizing that for this PR, and likely some others, we'll want to re-run the chromothripsis module with V22 first. I will do that now, so we can come back to this after the module itself has been updated for V22 in case results change.

jharenza commented 2 years ago

@sjspielman this is ready again. Post #1471, I reran the main figure 3 panel, updated the S3-BCD script to join on broad_histology + cancer_group and reran that script.

jharenza commented 2 years ago

Looks good! One requested change, something that I recently realized more generally and have been trying to update when I come across it. We hadn't previously set a seed with many of our jitter plots, like 3C-D, so these are not stable. Can you add a set.seed(whatever you want!) in that section of that scripts/supp-S3-panels-BCD.R, and re-run?

Done - I realized we do have one figure being copied over for Figure 3C, but I think we did not set seed in #1471 

cp ${analyses_dir}/chromothripsis/plots/04-breakpoint-data/count_chromothripsis_cnv_and_sv_breaks_scatterplot.pdf pdfs/fig3/panels/count_chromothripsis_cnv_and_sv_breaks_scatterplot.pdf

Do you want me to a) copy that over or b) set seed + rerun the whole module/copy over? c) else?

sjspielman commented 2 years ago

Do you want me to a) copy that over or b) set seed + rerun the whole module/copy over? c) else?

Oh I totally missed that one of those was copied over! Yes, definitely feel free to do that here; I dismissed my review. Edit: "that" means option "b." We can also do that in a separate PR, and I can re-approve this. Either way is ok with me.

jharenza commented 2 years ago

@sjspielman I reran just script 04 actually, which produces the plot, then copied over.

jharenza commented 2 years ago

Ahhh right!