AlexsLemonade / OpenPBTA-analysis

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

Change R^2 to R on SV vs. CNV breaks plot #1215

Closed LauraEgolf closed 2 years ago

LauraEgolf commented 2 years ago

Purpose/implementation Section

What scientific question is your analysis addressing?

Chromothripsis module - change R^2 to R on scatterplot of SV vs. CNV breaks (for consistency with other figures/text)

What was your approach?

What GitHub issue does your pull request address? #1214

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

Which areas should receive a particularly close look? N/A

Is there anything that you want to discuss further?

If this makes review easier, I could create a new branch and just copy the updated plot over to figures instead of re-running the figure generation script. (I just ran it to make sure everything worked)

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

LauraEgolf commented 2 years ago

Do you know why consensus_seg_recurrent_focal_cn_units.tsv had updates? I wasn't expecting any changes to other modules here.

I'm not sure why that file changed when I re-ran the figure generation script, but none of the other modules depend on the chromothripsis module (to my knowledge) so I don't think it had anything to do with the updates I introduced here. I expect it would probably look the same if you re-ran it on a clean branch off of master.

Not sure how you want to resolve that, but re-running the figure generation script isn't really necessary for this PR since all the script does is copy the updated scatterplot over. I ran it just to make sure that everything was up-to-date, but maybe it would be better if I move this minor change to a new branch and don't re-run figure generation?

sjspielman commented 2 years ago

Hi all, I'll have a look tomorrow about what might be going on with the changes in data. Stay tuned!

@LauraEgolf, can you confirm that the changes you are seeing occur when running the code within the Docker? We would expect some changes if a file is run in a different environment due to different package versions and operating system. There's of course also the chance the original data was generated outside of Docker, so by running properly in Docker you get the diff.

jaclyn-taroni commented 2 years ago

If I recall correctly, @LauraEgolf had some trouble getting Docker installed! It could be the difference between within the container vs. not.

sjspielman commented 2 years ago

Hi @LauraEgolf, I'd like to go into your branch here to fix the merge conflict and look into data discrepancies more in depth. I have addressed the merge conflict locally (in your branch), but I'm running into some permissions issues being able to push that fix over.

I wonder if you could check something for me - in order for me to be able to do this, this PR needs to have checked on "Allow edits from maintainers", which is something that has to be done on your end. Some more information about that is here. Can you check if this is turned on (and if not, please turn it on!), and let me know? Thanks!

LauraEgolf commented 2 years ago

I wonder if you could check something for me - in order for me to be able to do this, this PR needs to have checked on "Allow edits from maintainers", which is something that has to be done on your end. Some more information about that is here. Can you check if this is turned on (and if not, please turn it on!), and let me know? Thanks!

@sjspielman Looks like "Allow edits by maintainers" is already checked for this PR. Hmm, not sure what might be causing the permissions issue... should I add you to the fork as a collaborator?

sjspielman commented 2 years ago

How odd! It could be because I was not formally a maintainer when you filed the PR, but who knows! If you feel comfortable making me a collaborator on your fork that might solve it as a one-time solution, so let's try that. If no dice, I can file a PR to your branch specifically.

LauraEgolf commented 2 years ago

@sjspielman That might be why! I just added you as a collaborator - seems like the easiest solution.

sjspielman commented 2 years ago

Whew, made it! The issue was on my end I think (I was using my password instead of a PAT, the new way for https auth on github). Thanks for your patience!

sjspielman commented 2 years ago

I ended up making a couple more changes - updated figure theme to theme_pubr() and updates figures accordingly.

Now, to track down why that tsv is changing!

sjspielman commented 2 years ago

I can confirm the TSV differences are related to the data release. The current TSV file in master was run with V20, and changes in V21 data release will lead to changes in this TSV file. Because the figure regeneration script was run here, the TSV file was regenerated with V21 and we now see the difference.

Looping in @jaclyn-taroni with ideas for the best way to proceed here, since it's not clear to me we want to be updating other module results from this PR. We may want to deal with that separately.

jaclyn-taroni commented 2 years ago

analyses/focal-cn-file-preparation/results/consensus_seg_recurrent_focal_cn_units.tsv isn't included in the data download, so it seems fine to me to update it here.

sjspielman commented 2 years ago

Given @jaclyn-taroni's comment, I'm going to approve this PR and note explicitly that this PR updates analyses/focal-cn-file-preparation/results/consensus_seg_recurrent_focal_cn_units.tsv to be created with the v21 data release.

LauraEgolf commented 2 years ago

Thanks for resolving everything!