AlexsLemonade / OpenPBTA-analysis

The analysis repository for the Open Pediatric Brain Tumor Atlas Project
Other
100 stars 67 forks source link

EXTEND: re-analyze trends at tumor purity threshold #1657

Closed sjspielman closed 1 year ago

sjspielman commented 1 year ago

Closes #1625 This PR re-assess EXTEND scores if generated from only tumors that pass a tumor purity threshold. A couple steps are taken:

Everything is broadly consistent with our existing results, but I do find some areas we could rephrase during revisions anyways. Here is the notebook: 07_EXTEND-at-threshold.nb.html.zip

sjspielman commented 1 year ago

@jaclyn-taroni The tumor filtering function has now been incorporated here. I re-ran the module and there were no diffs in any results, so seems good as long as CI agrees 😄

jaclyn-taroni commented 1 year ago

@sjspielman reviewing now - I think something funky is happening with the HTML output that's committed/pushed at the moment. Specifically in the re-analysis 2 section and beyond.

sjspielman commented 1 year ago

@jaclyn-taroni I've fleshed out the docs with more information for these additions, and I added in some details about running the module in general. I'm glad I did this because some docs were missed in #1653 and that script (06) didn't get added to CI 😬 . Should be set now!! That said, I'll note more generally that this module's README could be expanded quite a bit in other ways that are beyond the scope of this PR, and in the interest of staying on task I avoided too many recreational changes there.

I also went back and forth a bit thinking about this comment https://github.com/AlexsLemonade/OpenPBTA-analysis/pull/1657#discussion_r1098928446, and I think that it really does make sense to always make sure that analysis is run. I moved the thresholding script/notebook out of the logic entirely so it'll always get called.

jaclyn-taroni commented 1 year ago

I don't believe you've resolved the issue with the HTML noted in https://github.com/AlexsLemonade/OpenPBTA-analysis/pull/1657#issuecomment-1421054716 at this point, @sjspielman.

sjspielman commented 1 year ago

I don't believe you've resolved the issue with the HTML noted in https://github.com/AlexsLemonade/OpenPBTA-analysis/pull/1657#issuecomment-1421054716 at this point, @sjspielman.

Woops, somehow missed your comment up there about the HTML when I went ahead to re-request review! Fixing it now. Sorry about that confusion! At some point I was testing locally with CI data, and it borked the notebook.

I am also a little confused about why the Fig 4 panels are being update here, which was probably something included earlier that I did not notice!

Hm, great question... Somehow they got opened along the way and VS Code ate all the EOL spaces are the diffs there, and then further somehow I missed it in git status. As to how PDFs got regenerated, well, who knows that's quite the mystery to me. A series of unfortunate events. I'll go ahead and revert all of those.......

sjspielman commented 1 year ago

Ok @jaclyn-taroni, feeling confident 🤞 that this is ready. Plots are reverted and here's the properly-rendered HTML: 07_EXTEND-at-threshold.nb.html.zip