Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

feat(technology): add Oxford Nanopore processing #305

Merged
merged 144 commits into from
Dec 2, 2021
Merged

feat(technology): add Oxford Nanopore processing #305

merged 144 commits into from
Dec 2, 2021

Conversation

thomasbtf
Copy link
Contributor

Expected commit message structure

Commit messages should be structured as follows:

<type>[optional scope]: <description>

[optional body]

[optional footer]

Type

Must be one of the following:

  • build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
  • ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
  • docs: Documentation only changes
  • feat: A new feature
  • fix: A bug fix
  • perf: A code change that improves performance
  • refactor: A code change that neither fixes a bug nor adds a feature
  • style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
  • test: Adding missing tests or correcting existing tests

Examples

Commit message with description and breaking change in body

feat: allow provided config object to extend other configs

BREAKING CHANGE: `extends` key in config file is now used for extending other config files

Commit message with no body

docs: correct spelling of CHANGELOG

Commit message with scope

feat(lang): added polish language

Commit message for a fix using an (optional) issue number.

fix: minor typos in code

see the issue for details on the typos fixed

fixes issue #12

thomasbtf and others added 30 commits October 11, 2021 17:10
@thomasbtf
add ont toolchain
edb88c1
@thomasbtf
move medka
a2be122
@thomasbtf
add primer clipping
2093baf
@thomasbtf
add env for primer clipping
c65c23b
@thomasbtf
integrate reads in common.smk
4192601
@thomasbtf
fmt
d367f34
@thomasbtf
add medaka_model to test config
b3893ab
@thomasbtf
update sample sheet
cb91ce0
@thomasbtf
add check for benchmark data
66d4ca7
@thomasbtf
fmt
c4f88b8
@thomasbtf
Merge branch 'master' into ont
e0981b2
@thomasbtf
Merge branch 'master' into ont
37cc85a
@thomasbtf
add ci for ont
f5266a2
@thomasbtf
change schema for new sample sheet
76293de
@thomasbtf
linting
2eb3c2c
@thomasbtf
add ARTIC_v3_adapters
cf5fc22
@thomasbtf
fmt
6bbb2ce
@thomasbtf
add ARTIC primers to resources
0ed4c42
@thomasbtf
integrate ont pipieline
7340b19
@thomasbtf
fmt
e2a23e4
@thomasbtf
add missing wildcards
a69b25a
@thomasbtf
agg is_sample_technongoly in is_technology
d425358
@thomasbtf
change ont assembly from canu to spades se
d5c7fcf
@thomasbtf
fmt
2e4cb84
@thomasbtf
make artic primers version adjustable
cfff553
@thomasbtf
adjust fastp, kranken and samtools depth
dcca93e
@thomasbtf
rename rules
1b73612
@thomasbtf
adjust kallisto
58204f9
@thomasbtf
fmt
6f57fb2
@thomasbtf
fmt
a0651df
@thomasbtf thomasbtf changed the title feat: add ont/longread processing feat(ont): add ont/longread processing Nov 29, 2021
@thomasbtf thomasbtf marked this pull request as ready for review December 1, 2021 15:36
@thomasbtf thomasbtf changed the title feat(ont): add ont/longread processing feat(technology): add ont/longread processing Dec 2, 2021
@thomasbtf thomasbtf changed the title feat(technology): add ont/longread processing feat(technology): add Oxford Nanopore processing Dec 2, 2021
alethomas
alethomas approved these changes Dec 2, 2021
View reviewed changes
Copy link
Collaborator

@alethomas alethomas left a comment

Choose a reason for hiding this comment

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

Overall a real heavy feature. Nice work

.github/workflows/main.yml
@@ -149,6 +149,7 @@ jobs:
mkdir -p .tests/data
curl -L https://github.com/thomasbtf/small-kraken-db/raw/master/B.1.1.7.reads.1.fastq.gz > .tests/data/B117.1.fastq.gz
curl -L https://github.com/thomasbtf/small-kraken-db/raw/master/B.1.1.7.reads.1.fastq.gz > .tests/data/B117.2.fastq.gz
curl -L https://github.com/thomasbtf/small-kraken-db/raw/master/ont_reads.fastq.gz > .tests/data/ont_reads.fastq.gz
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it be better to move those files to the official repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to avoid making this repo too big, space-wise. These files would be forever in the history of the repo, thus in each new "master-merge".

I would like to suggest this as a long-term solution: https://github.com/marketplace/actions/qc-benchmark-action

However, this is another big PR itself.

workflow/envs/spades.yaml
@@ -2,5 +2,5 @@ channels:
- bioconda
- conda-forge
dependencies:
- spades =3.15.2
- spades =3.15.3
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it makes sense to update such versions within a small, extra PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jep, you are right. However, the spades_se version only works with this (https://github.com/IKIM-Essen/uncovar/pull/305/files#diff-973dfb502bde3e2836e467b210d176f7b91c2b1b0dd8adde0628827af2527371R67), because of the naming of the output files. As I added this I just changed the env.

workflow/rules/assembly.smk
@@ -41,7 +41,7 @@ rule assembly_megahit:
" > {log} 2>&1"


Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking of separating the assembly for ONT and illumina. Are they sharing most of the steps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, they are already separated. One just has to expand the config and probably this function (https://github.com/IKIM-Essen/uncovar/pull/305/files#diff-973dfb502bde3e2836e467b210d176f7b91c2b1b0dd8adde0628827af2527371R181) to make the ONT assembler switchable. I also used the usuall contig paths to ensure, that we can expand the assembler comparison.

workflow/rules/common.smk
# clear text / content of flag "technology" in sample sheet
ILLUMINA = "illumina"
ONT = "ont"

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm really a little worried how this thing grew now. Many kind of similar names and loads of functions. Heavily
Overcrowded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agree. We have to refactor this soon. Maybe we can do this while addressing #357 ?

@thomasbtf thomasbtf merged commit 55f38f3 into master Dec 2, 2021
@thomasbtf thomasbtf deleted the ont branch December 2, 2021 18:42
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@alethomas alethomas alethomas approved these changes

@johanneskoester johanneskoester Awaiting requested review from johanneskoester

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace fasta parsa in mask-contigs.py Make pipeline oxford nanopore ready Include RKI quality criteria
3 participants
@thomasbtf @alethomas @simakro