-
Notifications
You must be signed in to change notification settings - Fork 709
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
Fix #1018 #1037
Fix #1018 #1037
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 think two of the if clauses should be rephrased slightly with AND conjunctions. !skip_peudoalignment
seems a bit too lenient to me.
if (params.pseudo_aligner) { prepareToolIndices << params.pseudo_aligner } | ||
if (!params.skip_bbsplit) { prepareToolIndices << 'bbsplit' } | ||
if (!params.skip_alignment) { prepareToolIndices << params.aligner } | ||
if (!params.skip_pseudo_alignment) { prepareToolIndices << params.pseudo_aligner } |
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 you should have put params.pseudo_aligner && !params.skip_pseudo_alignment
here, because params.pseudo_aligner
is null
by default?
So it will add null
to prepareToolIndices
most of the time. This may or may not cause issues, depending on how prepareToolIndices
is used later, but I think it preferable to avoid that in the first place?
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.
We are also doing parameter validation for --pseudo_aligner
and --aligner
via the Nextflow schema which should fail sooner before hitting this logic which is why I hadn't included it. But we can be explicit 👍🏽
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.
Done 993fff6
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.
We are also doing parameter validation for
--pseudo_aligner
and--aligner
via the Nextflow schema which should fail sooner before hitting this logic which is why I hadn't included it. But we can be explicit 👍🏽
Admittedly, those peculiarities escaped me. I just assumed that !params.skip_pseudo_alignment
will also evaluate to true
when no pseudoalignment is performed at all (because the user performs a regular run with alignment).
Closes #1018