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

Using mapping histology groups for plotting implementation (PR 5 of 5) #927

Merged

Conversation

cansavvy
Copy link
Collaborator

@cansavvy cansavvy commented Jan 25, 2021

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 #899

In this PR, I'm updating the figures/TelomeraseActivities.R script to use the display_group and hex_codes that are in palettes/histology_label_color_table.tsv. After I updated the script I re-ran it and pushed the updated plots.

What was your approach?

This PR is 5 of a few PRs where I am updating things in response to the table added in #898

What GitHub issue does your pull request address?

I think this should be the last one in this series for closing up #898 . Circle CI should tell us this because I have removed the old histology_color_palette.tsv file.

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

  • I hadn't looked at this code much before. Hopefully the intentions of the plots are still there.
  • The other barplots look kind of drab without colors?

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

Yes

Results

The Telomerase figure in the figures folder as generated by the TelomeraseActivities.R script is updated:

Telomerase_Activities

Reproducibility Checklist

  • The dependencies required to run the code in this pull request have been added to the project Dockerfile. -- no new packages are needed.
  • This analysis has been added to continuous integration.

Documentation Checklist

I checked the respective READMEs for these analyses and they did not require any updates for these changes.

  • 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.

@cansavvy cansavvy marked this pull request as draft January 25, 2021 18:21
@cansavvy cansavvy changed the title WIP: Using mapping histology groups for plotting implementation (PR 5 of 5????) WIP: Using mapping histology groups for plotting implementation (PR 5 of 5) Jan 26, 2021
@cansavvy cansavvy marked this pull request as ready for review January 26, 2021 15:13
@cansavvy cansavvy changed the title WIP: Using mapping histology groups for plotting implementation (PR 5 of 5) Using mapping histology groups for plotting implementation (PR 5 of 5) Jan 26, 2021
@cansavvy cansavvy requested a review from jashapiro January 26, 2021 19:44
Copy link
Member

@jashapiro jashapiro left a comment

Choose a reason for hiding this comment

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

This looks good from a simple standpoint of replacing short_histology with broad_histology, but I am going to guess that this figure needs more reworking. I expect this should happen in a separate issue, but the main thing I see is the following question:

Do we want both P1 and P2 (A&B) ? It seems to me that display_group and broad_histology are very similar, and we might not want to include both. Maybe we want to replace P1(A) with short_histology with all its messiness (and no colors?) as use display_group for P2(B) Or maybe we should add more plots that are like C covering other groups that have been split apart by subtyping?

I think I would probably lean toward doing my first suggestion for now: make A short_histology and B display_group.

This is probably a question for @jharenza, or perhaps one that should simply appear as a new issue once this PR is in (or both).

My other comments are style and a thought about reducing the uninformative significance bars.

@cansavvy cansavvy requested a review from jashapiro January 28, 2021 11:39
Copy link
Member

@jashapiro jashapiro left a comment

Choose a reason for hiding this comment

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

The code looks prettier now, thanks! For some reason styler missed one long line, so I fixed that in a comment.

Other than that, I think to preserve the original plot better, we should (for now) keep the upper (A) plot as short_histology (or even harmonized_diagnosis), which will now be potentially quite varied, and will certainly have more colors than we have. But we can still color them by display_group colors, which may make it easier for people to draw connections with plot B, which would be the smaller set of display groups, to parallel the previous broad histology plot.

@cansavvy
Copy link
Collaborator Author

(A) plot as short_histology (or even harmonized_diagnosis), which will now be potentially quite varied, and will certainly have more colors than we have. But we can still color them by display_group colors, which may make it easier for people to draw connections with plot B, which would be the smaller set of display groups, to parallel the previous broad histology plot.

I wasn't sure how to translate the display_group colors to the short_histology ones since its not a clean lining up of these groups. The other difference is I put display_group on the top and short_histology on the bottom.

Telomerase_Activities

@cansavvy
Copy link
Collaborator Author

Okay. Short_histology wasn't making much sense, so we've switch to harmonized diagnosis which is not bad. This does somewhat make Panel C redundant, but this is something we can revisit another time.

Telomerase_Activities

Copy link
Member

@jashapiro jashapiro left a comment

Choose a reason for hiding this comment

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

This looks good to me for now. There will definitely be some changes if this figure is used in the final, but I like that the current version shows a few different approaches that can be adapted in the future.

@jaclyn-taroni jaclyn-taroni merged commit 837db5c into AlexsLemonade:master Jan 31, 2021
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