-
Notifications
You must be signed in to change notification settings - Fork 83
Using mapping histology groups for plotting -- Part 2 implemention (PR 2 of 4) #911
Using mapping histology groups for plotting -- Part 2 implemention (PR 2 of 4) #911
Conversation
…ansavvy/mapping-table
…ansavvy/mapping-table
…ansavvy/mapping-table
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Just one comment because I don't trust the machines.
histologies_color_key_filtered <- unique(histologies$hex_codes) | ||
names(histologies_color_key_filtered) <- unique(histologies$display_group) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little worried about separate unique()
calls preserving order... I think it should work fine, but it seems somehow dangerous to me. So I present an alternative below.
histologies_color_key_filtered <- unique(histologies$hex_codes) | |
names(histologies_color_key_filtered) <- unique(histologies$display_group) | |
histologies_color_key_filtered <- histologies %>% | |
dplyr::select(display_group, hex_codes) %>% | |
dplyr::distinct() %>% | |
dplyr::pull(hex_codes, name = display_group) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I like this better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out the name argument for pull
is not in our version of dplyr for our docker image, so I can't use the last part, but I will add a distinct to this part but in my subsequent PR of this series.
Purpose/implementation Section
What scientific question is your analysis addressing?
This PR addresses Step 3 and part Step 4 discussed in #897 and #898. This PR is updating modules that should now be using the
palettes/histology_label_color_table.tsv
table that is created by the notebook added in #899What was your approach?
This PR is 2 of a few PRs where I am updating things in response to the table added in #898
Here's what's accomplished in this PR and what files have changes that need review (the rest are updated plots):
cnv-chrom-plot/cn_status_heatmap.Rmd
now uses the histology groupings and hex code colors from histology_label_color_table.tsvmutational-signatures/known_signature.Rmd
now uses the histology groupings and hex code colors from histology_label_color_table.tsvmutational-signatures/util
so that they useddisplay_group
instead ofshort_histology
for the plotting.I deleted the old plots, re-ran both of the notebooks mentioned above, and now am pushing the updated plots and results. That's where the crazy number of files changed comes from, a lot of plot updates.
*Note that the
palettes/histology_color_palette.tsv
file itself is not deleted yet, I will do this in the last PR of this series so the other modules that use this file don't break in CI yet.What GitHub issue does your pull request address?
Makes more progress toward addressing #898
Directions for reviewers. Tell potential reviewers what kind of feedback you are soliciting.
Which areas should receive a particularly close look?
Is the analysis in a mature enough form that the resulting figure(s) and/or table(s) are ready for review?
Yes. Here's updated cn status heatmap from cnv-chrom-plot:
And here's some of the updated plots from the known signature analysis in
mutational-signatures
(I did not included thesignature_grouped_barplots
folder of plots; it was too big.Results
What is your summary of the results?
Plots are different and now use the
display_group
histology labels instead ofshort_histology
.Note that cnv-chrom-plot does not use the histology color palette, but it does still use
short_histology
for its plots -- *I have not updated this notebook here, we should discuss if we should update that in another PR. *Reproducibility Checklist
Documentation Checklist
I checked the respective READMEs for these analyses and they did not require any updates for these changes.
README
and it is up to date.analyses/README.md
and the entry is up to date.