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

Tp53 loss status update: bug fix #942

Merged
merged 6 commits into from
Feb 17, 2021

Conversation

kgaonkar6
Copy link
Collaborator

@kgaonkar6 kgaonkar6 commented Feb 15, 2021

Purpose/implementation Section

What scientific question is your analysis addressing?

TP53 loss status was incorrectly coded in the previous PR where we converted some columns to binary but I didn't propogate the changes in the code correctly. I've now updated lines which caused the bug.

What was your approach?

Updating #922 to

  • replace NA with 0 only when merging the hotspot/activating status with the full histology
  • correctly summarizing and coding the conditions in TP53 status assignment chunk.

What GitHub issue does your pull request address?

#837

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

Which areas should receive a particularly close look?

The summarizing binary status of hotspot/activating from SNV/indels is currently set as 1 if any SNV/indel is in hotspot and 0 if non of the multiple SNV/indels are in hotspots to keep it binary.
Or should it be comma separated as 1,0 is HGVSp_Short=X,Y and X overlaps a hotspot and Y doesn't?

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?

Results

What types of results are included (e.g., table, figure)?

table

What is your summary of the results?

tp53_alterations_score_wide$tp53_altered %>% table()
activated      loss     Other 
       23       136       971 

Reproducibility Checklist

  • The dependencies required to run the code in this pull request have been added to the project Dockerfile.
  • This analysis has been added to continuous integration.

Documentation Checklist

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

@kgaonkar6 kgaonkar6 changed the title Tp53 lossbug fix Tp53 loss status bug fix Feb 16, 2021
Copy link
Collaborator

@jharenza jharenza left a comment

Choose a reason for hiding this comment

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

Thank you @kgaonkar6 ! The fix looks good. I just have one comment - For Check if other cancer predisposition have high TP53 inactivation scores display in the HTML file looks very smushed - is it rendering that way on your end as well? Maybe a rerun and save of the HTML would fix?

@kgaonkar6 kgaonkar6 added the ready for review Used to label pull requests that are ready for review label Feb 16, 2021
@kgaonkar6
Copy link
Collaborator Author

Thank you @kgaonkar6 ! The fix looks good. I just have one comment - For Check if other cancer predisposition have high TP53 inactivation scores display in the HTML file looks very smushed - is it rendering that way on your end as well? Maybe a rerun and save of the HTML would fix?

Thanks for the review @jharenza! I added larger size param in each of the chunks to enlarge the plots.

@kgaonkar6 kgaonkar6 changed the title Tp53 loss status bug fix Tp53 loss status update: bug fix Feb 16, 2021
Copy link
Collaborator

@jharenza jharenza left a comment

Choose a reason for hiding this comment

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

looks good now, thank you!

@jharenza
Copy link
Collaborator

I am going to merge this quick bug fix.

@jharenza jharenza merged commit 532c29a into AlexsLemonade:master Feb 17, 2021
@kgaonkar6 kgaonkar6 deleted the TP53-lossbug-fix branch May 13, 2021 17:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ready for review Used to label pull requests that are ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants