-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
parse README #6
parse README #6
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.
Several style, some content, and a few discussion points. Looks great :)
``` | ||
|
||
## **AFTER GENERATING A NEW REPO, CHANGE OR DELETE ALL NONSPECIFIC DETAILS** | ||
|
||
<p align="center"> | ||
<img src="https://raw.githubusercontent.com/broadinstitute/pooled-cp-profiling-template/a57cb7f9e36b89ff56acf094f18ca06b1a53b719/media/pipeline_weld.png" width="500"> | ||
</p> |
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.
We should add a figure legend. Do you want to give it a first crack?
@@ -0,0 +1,31 @@ | |||
The following are the two setup steps that need to be performed once at the start of a project. |
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.
The following are the two setup steps that need to be performed once at the start of a project. | |
The following are the two setup steps that need to be performed **only once** at the start of an experiment. |
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 think of each batch as possibly being a separate experiment, and these steps don't need to happen with each batch. So if you don't like "project" and I don't like "experiment", can we come up with another word?
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.
And/or we should put into the documentation how we define project/experiment/batch?
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.
Want to say "start of each batch of data collection" or "start of each project batch"? Let's keep only once though.
I agree the data pipeline welding "unit" will be experimental batch. An experiment may contain multiple batches, and a project may contain multiple experiments (in my view).
Although we may eventually want to make the recipe focused at the "experiment" level (as defined above) since we are likely to want to develop batch effect correction methods. These tools should be part of the recipe IMO - this is a bit down the road though
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 just added a commit that defines our terms and so it should be consistent now.
I agree we want to eventually bring it to the experiment level (related issue).
@@ -0,0 +1,104 @@ | |||
The following are the weld process steps to perform with each dataset you analyze. | |||
|
|||
For a general overview of the pipeline welding process, see the [repo README](README.md). |
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.
Love these pointers
weld_process_README.md
Outdated
# Add, commit, and push the submodule contents | ||
git add pooled_cp_profiling_recipe | ||
git add .gitmodules | ||
git commit -m 'finalizing the recipe weld' |
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.
whatever word we decide above should be substituted in this commit message. It was probably me that used this terminology before 😅
Co-authored-by: Greg Way <gregory.way@gmail.com>
@gwaygenomics Take another look at your convenience. I think I've addressed everything in your review and our conversation. 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.
minor comments, LGTM
Co-authored-by: Greg Way <gregory.way@gmail.com>
Co-authored-by: Greg Way <gregory.way@gmail.com>
No description provided.