-
Notifications
You must be signed in to change notification settings - Fork 83
Splitting up #921: Immune-deconv changes #929
Splitting up #921: Immune-deconv changes #929
Conversation
@@ -29,13 +29,9 @@ deconvout <- opt$input | |||
output <- opt$output | |||
load(deconvout) | |||
|
|||
# if short histology is Medulloblastoma or ATRT, use them as broad histology |
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 didn't think this was relevant anymore so I deleted this step completely, but someone should let me know if something like this is still needed.
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.
confirmed this is ok to delete with the switch to harmonized dx
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.
The changes made to the scripts in this PR look good to me!
It appears that parts of the heatmap_MCPcounter.pdf
and heatmap_xCell.pdf
plots have been cutoff so the sizing will likely need to be adjusted.
As for your point in this PR's original comment:
I have more questions about why we are keeping CIBERSORT based methods since we can't re-run this (and haven't for almost a year). (We don't have access to the CIBERSORT.R script needed for this)
I also had the same question so I took a look at the README
...
Per this module's README.md
,
"We chose CIBERSORT (abs.) over MCP-Counter as it has the greatest number of overlapping immune cell types with that of xCell".
Also stated that,
"We recommend using MCP-Counter instead of CIBERSORT (abs.) when these files are not available to the user".
That said, I would argue that if this PR in particular goes in as is, we should update the README.md
to reflect the change in method used.
Yes. I saw these recommendations in the README. This is part of my question. Why are we keeping this around if it isn't something we can run? It might be something for a separate issue. On the separate note, I did notice the plots are trimmed badly, I'll look into fixing that. |
…-part2' into cansavvy/split-up-hist-color-3-part2
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.
With issue #931 filed, this PR LGTM! 👍
Purpose/implementation Section
What scientific question is your analysis addressing?
This PR is a split off of #921 where I am updating the transcriptomic overview plot in response to the histology color palette table added in #898
This PR updates immune-deconv module to use the display group and hex codes of the
palettes/histology_label_color_table.tsv
table that is created by the notebook added in #899What was your approach?
This PR updates one of the three modules that are ultimately used in the transcriptomic-overview plot that is ultimately updated in #921.
immune_deconv
updates:analyses/immune-deconv/01-immune-deconv.R
need to be updated to usedisplay_group
instead ofshort_histology
.analyses/immune-deconv/02-summary-plots.R
in the same wayOPENPBTA_DECONV_METHOD="mcp_counter" ./analyses/immune-deconv/run-immune-deconv.sh
(The CIBERSORT.R Doesn't work unless you have that CIBERSORT.R script which we do not).*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 (#927) 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
Closes #926
Directions for reviewers. Tell potential reviewers what kind of feedback you are soliciting.
Is the analysis in a mature enough form that the resulting figure(s) and/or table(s) are ready for review?
Yes?
Results
The results themselves don't really change here but will change in transcriptomic-overview.
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.