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

bwa-mem2 incorrect index #522

Open
lczech opened this issue Jul 13, 2022 · 6 comments
Open

bwa-mem2 incorrect index #522

lczech opened this issue Jul 13, 2022 · 6 comments
Labels
bug Something isn't working

Comments

@lczech
Copy link

lczech commented Jul 13, 2022

The wrapper 1.7.0 for bwa-mem2 contains the following code:

index = snakemake.input.get("index", "")
if isinstance(index, str):
    index = path.splitext(snakemake.input.idx)[0]
else:
    index = path.splitext(snakemake.input.idx[0])[0]

which mixes index and idx, clearly a bug.

PS: The command uses samtools sort, which would benefit from offering a temp directory.

@christopher-schroeder
Copy link
Contributor

@dlaehnemann @johanneskoester @tdayris
Blame shows you as author of this snippet. I can modify / fix that, but I don't see the point having this at all. bwa mem 2 requires a list of files as index. Allowing a single string implies that the user may only provide one of the index files.
It would be even cleaner if we require input.ref (fasta file) and input.index (all the index files).

@christopher-schroeder
Copy link
Contributor

christopher-schroeder commented Jul 19, 2022

I slept on it and realised that I'm going to modify the wrapper for cram output anyway. Since samtools sort needs the reference, input.ref makes sense. I am still against the "allow only one of the index files".
A rule should request all the files it needs to execute its command. If we start allowing only parts of it and imply that the rest is available, this could backfire badly in the future.

@fgvieira
Copy link
Collaborator

I agree, all input files needed by the rule should be explicitely specified.

@dlaehnemann
Copy link
Contributor

Absolutely agreed, that all necessary input files should be required. That was even part of the original PR that got this snippet in there, but that buggy snippet slipped through review, sorry. The only part that was adjusted to require all index files was the example Snakefile...

Copy link
Contributor

github-actions bot commented Nov 1, 2023

This issue was marked as stale because it has been open for 6 months with no activity.

@github-actions github-actions bot added the Stale label Nov 1, 2023
@lczech
Copy link
Author

lczech commented Nov 1, 2023

Bump to remove stale label.

@github-actions github-actions bot removed the Stale label Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants