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

Add bwa #103

Merged
merged 7 commits into from
Nov 8, 2024
Merged

Add bwa #103

merged 7 commits into from
Nov 8, 2024

Conversation

MarieLataretu
Copy link
Collaborator

No description provided.

relates to Replace minimap2 with another mapper for short-read data? #102
@hoelzer
Copy link
Member

hoelzer commented Aug 19, 2024

Nice! Looks good, I just built a new container w/ the latest bwa and samtools fixed to v1.20:

RUN conda install bwa=0.7.18 samtools=1.20 && conda clean -a

--> docker pull nanozoo/bwa:0.7.18--0eff897

so it matches the conda environment!

If we can have this in CLEAN soonish, we can also update the mnsc accordingly before re-submission.

@MarieLataretu MarieLataretu marked this pull request as ready for review August 20, 2024 15:00
Copy link
Collaborator

@matthuska matthuska left a comment

Choose a reason for hiding this comment

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

I didn't run the updated pipeline but the changes look reasonable. I'll leave testing on real data to yourself and @hoelzer

modules/bwa.nf Outdated Show resolved Hide resolved
tests/illumina/main.nf.test Outdated Show resolved Hide resolved

script:
"""
INDEX=`find -L ./ -name "*.amb" | sed 's/\\.amb\$//'`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't the index passed in as a parameter (db_index)? If you only want to strip off the .amb you can use something like:

index_root=${db_index%.amb}

(not tested)
see https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The BWA processes are copied from nf-core 😇

The index process outputs a folder called bwa, and with this line, you find the folder. Maybe amb is BWA specific? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this is a Nextflow thing, but vertically aligning curly braces makes for unnecessarily larger diffs with meaningless changes. I'd consider it bad style but that's just me. I can ignore them in GitHub but it's more of a pain when using command line git (e.g. git blame especially).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@hoelzer
Copy link
Member

hoelzer commented Aug 20, 2024

Great! I would like to test it “real live” on the data where I also saw such discrepancies between BBDUK and minimap2 #102. I will do that in the next few days and report

bwa mem \\
-t $task.cpus \\
\$INDEX \\
$input \\
Copy link
Member

Choose a reason for hiding this comment

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

bwa mem does automatically take care of single-end or paired-end read input, or? So you just pass $input and then this will be either a SE FASTQ or two PE FASTQs, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, for single-end, it would be one fastq file; for paired-end, $input would be foo_1.fastq foo_2.fastq (space-separated)

@hoelzer
Copy link
Member

hoelzer commented Aug 23, 2024

Did you use bwa index -a bwtsw? See the man page, it seems to be necessary for human genome sized indexes: https://bio-bwa.sourceforge.net/bwa.shtml

@hoelzer hoelzer merged commit 9114a4e into dev Nov 8, 2024
10 of 12 checks passed
@hoelzer hoelzer deleted the add_bwa branch November 8, 2024 15:00
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.

3 participants