Skip to content
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

Merged
merged 6 commits into from
Jul 28, 2020
Merged

parse README #6

merged 6 commits into from
Jul 28, 2020

Conversation

ErinWeisbart
Copy link
Member

No description provided.

Copy link
Member

@gwaybio gwaybio left a 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>
Copy link
Member

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Member Author

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?

Copy link
Member Author

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?

Copy link
Member

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

Copy link
Member Author

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

setup_README.md Outdated Show resolved Hide resolved
weld_process_README.md Outdated Show resolved Hide resolved
@@ -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).
Copy link
Member

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 Show resolved Hide resolved
weld_process_README.md Outdated Show resolved Hide resolved
weld_process_README.md Outdated Show resolved Hide resolved
# Add, commit, and push the submodule contents
git add pooled_cp_profiling_recipe
git add .gitmodules
git commit -m 'finalizing the recipe weld'
Copy link
Member

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 😅

weld_process_README.md Outdated Show resolved Hide resolved
@ErinWeisbart
Copy link
Member Author

@gwaygenomics Take another look at your convenience. I think I've addressed everything in your review and our conversation. Thanks!

Copy link
Member

@gwaybio gwaybio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comments, LGTM

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
ErinWeisbart and others added 2 commits July 28, 2020 11:47
Co-authored-by: Greg Way <[email protected]>
Co-authored-by: Greg Way <[email protected]>
@ErinWeisbart ErinWeisbart merged commit 9e37287 into broadinstitute:master Jul 28, 2020
@ErinWeisbart ErinWeisbart deleted the readme branch August 24, 2020 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants