This repository has been archived by the owner on May 13, 2020. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Dev #2
base: master
Are you sure you want to change the base?
Dev #2
Changes from 7 commits
01d6e27
47b3d64
a2bd777
779a214
84d4ed8
09c27f2
c79165e
9244b37
fda80ad
0f5332c
56113ea
11f1870
7b4ae3a
97c96b3
6542dae
82da3a0
fb56dd4
197d34d
7db1cb2
8a1167c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is this just for WGS?
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.
According to the tutorial it should be set to 1000 for WGS and 0 for exomes. I noticed the default value for the task that uses the variable (PreprocessIntervals) has a default value for the variable of 1000. Since this workflow is directed towards exomes should the input json have this variable set to 0? @samuelklee
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.
You can technically use it for WES as well (to break targets up into smaller bins), we just haven't evaluated it. Let's leave the documentation unchanged, but set the default to 0 in the JSON---thanks for catching that, @bshifaw!
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.
Again, I never recommend using
latest
.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.
dict -> dictionary
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.
fai -> index
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.
Again, humans and based on our panel size.
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.
Important
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.
Something hinky happened with the copy around
thee
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.
This needs to be populated.
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.
Science Platforum -> Sciences Platform. Perhaps check the original text this was copied from and correct other instances of it? (Maybe just a final spell check would be worthwhile---and of course, we should correct any typos in the original workflows!)
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.
Does the license information not go in the beginning of each individual WDL?
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.
Yes it should be, but not sure why its missing for this repo. I'll add it now. As far why this section is here, its a note to inform the user that there might instance where a workflow will use none GATK tools and it's up to them to determine whether they can use it for their use case.