Preps QuaC for public availability
Merge request
Please fill in the checklist below and comment as needed.
-
Was code modified? Briefly describe. Yes, see below -
Was documentation modified? Briefly describe. Yes, see below -
Is this a bug-fix? Briefly describe. No -
Is this a feature addition? Briefly describe. Yes, see below -
Did you modify QuaC-Watch config file? If so, did you modify multiqc template configs/multiqc_config_template.jinja2
and scriptsrc/quac_watch/create_mutliqc_configs.py
as necessary? No, config file unmodified -
Did you perform system-level testing manually as described in master readme doc? Did it pass completely? If not why? Yes -
Updated Changelog.md
file with change logs in recommended format?
Anything else reviewer should know?
As part of making QuaC publicly available, following updates were made to make it more generic to the environment and user friendly:
- Removes prerun QC from small variant caller pipeline as requirement to QuaC (closes #45 (closed))
- Explicitly defines conda environments (closes #49 (closed))
- Uses container solution for
covviz
installation instead of conda to avoid pip based installation (closes #52 (closed)) - Removes git submodules and instead saves their local copy to repo (closes #53 (closed))
- Loads singularity module loading prior to executing the runner script
- Uses minimal snakemake instead of full-featured snakemake (closes #56 (closed))
Merge request reports
Activity
requested review from @bwilk777
- Resolved by Brandon M Wilk
- Loads singularity module loading prior to executing the runner script
@manag could you expand on what this statement describes? I can see where things were deleted from the run script generate in the run script but I don't see where this was changed elsewhere in the updates and don't quite understand what the change was.
- Resolved by Brandon M Wilk
This is totally not necessary and at your discretion but you may want to consider removing the hard coded table of contents at the top of the README because maintaining the links every time a section header changes is fraught with error. I didn't know this until recently but both GitLab and GitHub automatically provide table of contents for all READMEs (see this post about it ) in the same way. This isn't a "right in your face kind of replacement" as you need to know to go looking for it. GitLab has added support for the
[TOC]
tag in markdown to render these for you but GitHub hasn't yet (see my test repo for this) so there's no direct auto-generated solution to replace it as-is.
- Resolved by Brandon M Wilk
I can't highlight the lines where this is since they haven't changed in this MR
😅 but could you replace the hard coded value$USER_SCRATCH
from the various parts of QuaC? It wouldn't be portable to other systems that don't utilize that environment variable.Along those lines I also noticed that there are lots of Cheaha paths in headers and other files (like https://gitlab.rc.uab.edu/center-for-computational-genomics-and-data-science/sciops/pipelines/quac/-/blob/master/.test/ngs-data/test_project/analysis/A/qc/dedup/A-1.metrics.txt for example) that I'm thinking might be worth scrubbing for the sake that it's just testing data.
added 3 commits
added 1 commit
- 9d57f1e9 - removes mkdir for tmpdir as recent refactoring delegated this task to the user
- Resolved by Manavalan Gajapathy
@bwilk777 MR ready for you again :)
mentioned in commit 92db1d14