-
Notifications
You must be signed in to change notification settings - Fork 70
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
Move documentation to website #737
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.
I would rename all these files, ideally, following camel or snake casing (camel is safer as underscore may cause issues as you may need to escape it), and limit the use of dot separation for file type extensions only.
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.
Thank you, @mwalker174! This is a decent-sized PR with room for improvement. While it may make reviewing and tracking changes in follow-up PRs more challenging, given the time constraints, let's move forward with it now and plan to refine it in future iterations.
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.
Thanks for doing all of this! It will be great to have the website set up as a one-stop-shop for comprehensive documentation, with a more flexible layout than the README or the dashboard.
I launched the website and clicked through each page. The overall flow generally makes sense and I didn't notice link errors.
I tried to keep my comments simple and/or focused on essential questions/corrections with the plan to follow up with another PR proposing additional changes. I admit that my eyes glazed over a bit reading the inputs/outputs so I may not have gone through all of them thoroughly yet - I will do so, but wanted to get my initial review posted in a timely manner.
As mentioned offline, we should sync regarding near-term plans for ApplyManualVariantFilter and VisualizeCnvs.
I think the biggest change that's still needed here is a reorganization of the Terra execution documentation, as noted in comments.
website/docs/modules/main_vcf_qc.md
Outdated
[bcftools_preprocessing_options](#optional--bcftools_preprocessing_options) argument to limit plotted variants based on `FILTER` status. | ||
For example, to limit to `PASS` variants for a VCF generated from [FilterGenotypes](./fg) use: | ||
``` | ||
"bcftools_preprocessing_options": "-e 'FILTER~\"UNRESOLVED\" || FILTER~\"HIGH_NCR\"'" |
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.
Once #736 is merged this can just select PASS
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'll change it then
slug: viz | ||
--- | ||
|
||
Generates plots of read depth across all samples. This is useful for visualizing large deletions and duplications |
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.
Are we adding this to Terra? Is the version on the main branch ready?
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 it's ready and fairly critical.
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.
Are there important changes on eph-scale-vis-cnvs
other than massive scaling?
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 took a look. The other main change is visualizing CNVs within complex SVs, which is probably not necessary for 1.0
@@ -1,6 +1,6 @@ | |||
{ | |||
"label": "Docker Images", | |||
"position": 7, | |||
"label": "Docker builds", |
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.
"label": "Docker builds", | |
"label": "Docker Builds", |
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.
Using lower case on all the other pages so I'm going to leave this alone for now
``` | ||
{ | ||
"sample_1": { | ||
"good_variant_ids": ["variant_1", "variant_3"], | ||
"bad_variant_ids": ["variant_5", "variant_10"] | ||
}, | ||
"sample_2": { | ||
"good_variant_ids": ["variant_2", "variant_13"], | ||
"bad_variant_ids": ["variant_8", "variant_11"] | ||
} | ||
} | ||
``` |
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.
``` | |
{ | |
"sample_1": { | |
"good_variant_ids": ["variant_1", "variant_3"], | |
"bad_variant_ids": ["variant_5", "variant_10"] | |
}, | |
"sample_2": { | |
"good_variant_ids": ["variant_2", "variant_13"], | |
"bad_variant_ids": ["variant_8", "variant_11"] | |
} | |
} | |
``` | |
```json | |
{ | |
"sample_1": | |
{ | |
"good_variant_ids": ["variant_1", "variant_3"], | |
"bad_variant_ids": ["variant_5", "variant_10"] | |
}, | |
"sample_2": | |
{ | |
"good_variant_ids": ["variant_2", "variant_13"], | |
"bad_variant_ids": ["variant_8", "variant_11"] | |
} | |
} | |
``` |
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 is very cool
|
||
#### `*_vcfs` |
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 may override the section ids as {#vcfs}
to simplify referencing it (you would not need to escape _
and avoid asterisk).
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 haven't been totally consistent with this, but for most pages I left the identifiers alone to avoid verbosity.
Thanks for your reviews, just pushed changes @epiercehoffman @VJalili |
website/docs/execution/joint.md
Outdated
AF annotation with external population callsets | ||
|
||
Extra workflows (Not part of canonical pipeline, but included for your convenience. May require manual configuration): | ||
* `PlotSVCountsPerSample: Plot SV counts per sample per SV type | ||
* `MainVcfQc`: Generate detailed call set QC plots | ||
* `PlotSVCountsPerSample`: Plot SV counts per sample per SV type | ||
* `FilterOutlierSamples`: Filter outlier samples (in terms of SV counts) from a single VCF. Recommended to run | ||
* `PlotSVCountsPerSample` beforehand (configured with the single VCF you want to filter) to enable IQR cutoff choice. |
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.
* `PlotSVCountsPerSample` beforehand (configured with the single VCF you want to filter) to enable IQR cutoff choice. | |
`PlotSVCountsPerSample` beforehand (configured with the single VCF you want to filter) to enable IQR cutoff choice. |
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.
Ok I see another problem here which is maybe what you meant
Start working on modules Add best practices, troubleshooting link, more workflows Resource file documentation Add Docker images to getting started section Continue adding modules to documentation Docs through cleanvcf Hard filter and join raw calls Start filtergenotypes Finish filtergenotypes Finishing touches Add missing images Finish VisualzeCnvs page
9bfd712
to
76a5bab
Compare
Thanks, @mwalker174!! This is a most needed extension to the docs. |
Addresses #730.
The README is updated, but I have left the Terra dashboards intact for now until we can get a live link to the website.