-
Notifications
You must be signed in to change notification settings - Fork 83
PBTA Histologies: Fusion summary base (4 of N) #866
PBTA Histologies: Fusion summary base (4 of N) #866
Conversation
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 - 8 samples added to the summary files.
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 like this PR still needs a README update.
fusion-summary doesn't read in pbta-histologies file so I haven't updated the README, do you want to have an update in the module specifying to re-run per release? |
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.
My review comments so far here are pretty similar to part 3's review #865 (review)
But at this point I'm having trouble figuring out what is new to this PR as differentiated from part 3. We've accumulated enough changes that its a bit tricky to review and keep track.
Might be better to revisit this after v18 gets in and parts 1-3 are merged. Or if you can point out and summarize the specific file changes, that could help too.
analyses/fusion_filtering/05-QC_putative_onco_fusion_dustribution.Rmd
Outdated
Show resolved
Hide resolved
Thanks for the reviews @cansavvy. Since we had to update multiple modules to run for base subtyping I'm specifying the modules being re-run of updated in the "What scientific question is your analysis addressing?" let me know if it would be better to specify it clearly some other way. The only changes from this PR are: |
…OpenPBTA-analysis into fusion_filtering_base
…OpenPBTA-analysis into fusion_summarry_base
Thanks @cansavvy! These are tricky, but... these PRs (1-7) should be reviewed before V18 release is merged, because the final result of all of these is the new histologies file that will be released in V18, plus some of the summary files (fusion summary, fusion putative oncogenic, fusion recurrent, RNA-Seq collapsed). It is a tad bit circular, but the most straightforward way we could think to do these updates within OpenPBTA since we had been doing them on the backend pre-each release. I found that the easiest way to spot changes was to go into the module titled in each PR, then look for new file change dates and then review those. Sorry this is a bit confusing. I think we can also take your suggestion and point out the changes, too! |
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.
Now that part 1-4 are in, I can see that this is mostly (except one line) results changes which is what we'd expect based on samples being added. 👍
I just have one question about the non-results line add since I wasn't expecting it but this looks good to me.
@@ -106,6 +106,7 @@ Rscript 03-Calc-zscore-annotate.R --standardFusionCalls "${scratch_path}/standar | |||
--normalExpressionMatrix $normal_expression_file \ | |||
--outputfile "${scratch_path}/standardFusionStrandedExp_QC_expression" | |||
|
|||
if [ "$RUN_FOR_SUBTYPING" == 0 ]; then |
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 wasn't expecting this add. Is it something that was meant to be added previously but didn't get in?
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.
hmm this script is not part of fusion_summary module but would have been part of #865 which is already merged now. I'll revert these analyses/fusion_filtering/* files to master.
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.
ok reverted those files
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.
Just double checking: These changes are all intentional/what we want?: ca25c8d ?
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.
Looking back at your description, I think yes. I think we are all set. 4 results files changes and the changes to the nb.html is what we have currently.
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.
correct, thanks for double checking these are intentional to update to the module in master .
I must have missed merging the new changes from fusion_filtering-base to this branch which left these older file in this branch which was staggered from fusion_filtering_base branch.
Purpose/implementation Section
What scientific question is your analysis addressing?
fusion-summary
results/fusion_summary_embryonal_foi.tsv
(included in data download)results/fusion_summary_ependymoma_foi.tsv
(included in data download)results/fusion_summary_ewings_foi.tsv
results/fusion_summary_lgat_foi.tsv
What GitHub issue does your pull request address?
#861
Directions for reviewers. Tell potential reviewers what kind of feedback you are soliciting.
Which areas should receive a particularly close look?
This is only a re-run for v18 RNAseq samples other modules that need to be rerun for subtyping are in #861
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)?
tables
What is your summary of the results?
8 new samples were added to output files from this module
fusion_summary_embryonal_foi.tsv
fusion_summary_ependymoma_foi.tsv
fusion_summary_ewings_foi.tsv
fusion_summary_lgat_foi.tsv
Reproducibility Checklist
Documentation Checklist
README
and it is up to date.analyses/README.md
and the entry is up to date.