AlexsLemonade / OpenPBTA-analysis

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

survival analysis README added #1257

Closed runjin326 closed 2 years ago

runjin326 commented 2 years ago

Purpose/implementation Section

What scientific question is your analysis addressing?

Add REAME.md for survival analysis module

What was your approach?

Added descriptions for all 3 notebooks in the module. Modified the description in survival-analysis_histology.Rmd for clarification

What GitHub issue does your pull request address?

https://github.com/AlexsLemonade/OpenPBTA-analysis/issues/1249

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

Which areas should receive a particularly close look?

Please check to see whether the description makes sense.

Is there anything that you want to discuss further?

NA

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)?

NA

What is your summary of the results?

NA

Reproducibility Checklist

Documentation Checklist

sjspielman commented 2 years ago

Hi @runjin326 , looks good! A couple suggestions throughout.

The main other thing that needs to be done is adding the survival-analysis module to the overall analysis README (the one unchecked item in the initial PR comment).

Noting also - since we are going to be added more survival analyses, it will be important to again update both READMEs with the added results, plots, etc.

runjin326 commented 2 years ago

@sjspielman - thanks for the review. I modified the README.md file in analysis module now.

sjspielman commented 2 years ago

Noting I am blocking this by PRs #1264 and #1276 since these are going to modify the survival analysis module, and associated README changes will be required once they are merged. We should merge those first and then come back to solidifying this README.

jharenza commented 2 years ago

@sjspielman I will pick this up

sjspielman commented 2 years ago

Something a little wild seems to have happened here. There are now 207 file diffs well beyond the scope of this analysis. I suspect this is because the branch is not up to date?

One of two things should happen:

  1. Figure out why there are 207 file diffs (really there should only be 1 - the readme) and revert and/or update as needed so there is only 1 diff
  2. Close this PR and open a fresh one with just README changes.

The latter approach seems easier to me.

jharenza commented 2 years ago

I'll do 2. I almost did that before I worked on it, too.

sjspielman commented 2 years ago

Closing this PR in favor of a fresh clean one.