-
Notifications
You must be signed in to change notification settings - Fork 83
Conversation
recode "loss" values as "Del" and "gain" values as "Amp"
Hi @cbethell - I think this looks great for Amp/Dels. I did notice (especially for LGG From this issue, I had added:
To clarify - the |
…alysis into cbethell/recode-oncoprints
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.
As noted in the comment, if we can update the reciprocal fusions to not be multi-hit, I think that would take care of the rest of this PR.
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.
Hi @cbethell! Thanks for updating this!
I think the output looks exactly like we expect now and I did run the code and do some spot checking, and all looks good with your logic. I just made a small suggestion about shortening one of the dataframes.
I also noticed that all_participants_primary_only_oncoprint.png
and all_participants_primary-plus_oncoprint.png
are still in the code/plot output. Either we should keep them and update this new fusion code to be rendered in those plots or remove that piece of the code. I think that removing them should be fine.
Otherwise, I think this is almost set!
remove `all_participants` plots
@jharenza, thanks for the re-review! Although the updated code was not quite ready as I noticed instances where there are more than one I also could not locate where |
I think something happened to the reciprocal fusions in this latest commit in that I think we are missing a lot of them. I have been using LGG as a guide (eg in which we should see upwards of 50% of these samples harboring
I see they are now removed - I think the code was still in the bash script, but don't see it now. Thanks! |
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.
See the comment about the reciprocal fusions gone missing after last update.
Ahh yes great catch @jharenza, after some combination of the code in the previous commits, I believe the logic now produces what we would expect and we appear to have the reciprocal fusions back while retaining the true multi-hit fusions in 8cec80d (after using LGG as a guide and some further spot checking, this seems to be the case at least)! Let me know if you agree or if it appears that we may be missing some data -- I've also refactored a bit in the last set of updates so please feel free to comment on the structuring there! |
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.
Thanks for updating this @cbethell. I don't know why I didn't think of this before, but @kgaonkar6 had created a function in fusion_filtering
to identify reciprocal fusions and in fact, that populates the reciprocal_exists
column in the putative oncogenic file. So, if you want, you can leverage that column to possibly shorten that code. Up to you!
Otherwise, the outputs look good and I spot checked the few MET multi-hits from HGG and they look as expected.
I'll take a look at the code you linked, thanks @jharenza! |
Unfortunately, using the |
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.
- add `amplification` value when coding `gain` as `Amp` - re-run with all recent updates
I've addressed your comment about recoding the CNAs in 54e5b57 @jaclyn-taroni and re-ran this PR with said change and the merged changes from #1063. That said, I believe the plots now look as we expect so this is ready for a re-review. |
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 took a look at some of the plots that were discussed earlier in the thread and I get the sense that they look as expected 👍🏻
I'm going to merge this now, since there are a number of outstanding oncoprint PRs that will need to be updated! |
This reverts commit ea4f636.
Purpose/implementation Section
What scientific question is your analysis addressing?
Per #981 (comment), this PR recodes the
gain
andloss
values asAmp
andDel
, respectively, for the purpose of taking advantage of default maftools behavior that requires this coding when plotting.What was your approach?
This PR recodes the CNV file's
gain
andloss
values asAmp
andDel
within the01-plot-oncoprint.R
script of theoncoprint-landscape
module.What GitHub issue does your pull request address?
This PR closes #981 as it allows for better visualization of the
Multi_Hit
instances.Directions for reviewers. Tell potential reviewers what kind of feedback you are soliciting.
Which areas should receive a particularly close look?
The updated oncoprint plots in
oncoprint-landscape/plots
should receive a close look, probably from @jharenza, to ensure that this is the behavior we expect and want.Is there anything that you want to discuss further?
I will note that upon recoding, I updated the
oncoprint_color_palette.tsv
file infigures/palettes
to match the new coding. I also removed theamplification
value from this file as it did not appear in any of the previous oncoprints and would conflict with the new coding.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)?
No new results, just the broad histology oncoprints in
oncoprint-landscape/plots
were updated.What is your summary of the results?
Based on the example left in #981 (comment), the plots appear to now reflect the behavior we would want.
Reproducibility Checklist
Documentation Checklist
README
and it is up to date.analyses/README.md
and the entry is up to date.