Update pipeline summary report #644
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
d4straub
commented
Oct 12, 2023
d4straub
commented
Oct 12, 2023
erikrikarddaniel
approved these changes
Oct 13, 2023
| @@ -894,7 +892,7 @@ invisible(dev.off()) | |||
| cat("## DADA2\n") | |||
|
|
|||
| # indicate reference taxonomy | |||
| if (!params$flag_dada_ref_tax_user) { | |||
| if ( !isFALSE(params$dada2_ref_tax_title) ) { | |||
There was a problem hiding this comment.
This looks odd to me -- negating isFALSE instead of testing for true -- but maybe it's the way it should be?
(In (at least) one more place further down.)
There was a problem hiding this comment.
True that looks odd. I actually havent tested isTRUE but used !isFALSE because the default of that params is FALSE, if data is coming through it could be anything, TRUE, a string (file, setting, ...), a number,...
So I figured !isFALSE is my safest bet. Am I wrong?
There was a problem hiding this comment.
I've never used either of the two functions, but isTRUE would be my choice even if the default for the param is false. The difference between if ( isTRUE(cond) ) and if ( cond ) appears to be that isTRUE doesn't return NAs. isFALSE is defined analogously according to the documentation. isTRUE('string') returns false while evaluating a string as a logical returns NA. If you're not certain of what type the variable is (e.g. a logical or a string) I think you need to do something more elaborate.
There was a problem hiding this comment.
Yes, the point is here that I'm not certain of what type the variable is (e.g. a logical or a string) and
!isFALSE('string')
!isFALSE(TRUE)
!isFALSE(1)
all return TRUE, so seem to work as intended.
if ( 'string' ) {} returns Error in if ("string") { : argument is not interpretable as logical
isTRUE('string') is FALSE, so not doing what I want.
So while I agree the double negation looks bad, it seems to do what it should and the cleaner versions I came up with didnt.
There was a problem hiding this comment.
I am open to suggestion ofc ;)
There was a problem hiding this comment.
I suppose the correct thing would be to make sure the variable is logical before using it. I understand if you have better things to do than correct working code though.
There was a problem hiding this comment.
I have considered giving additional logical params to the Rmd script, but that did blow up the list of params tremendously and make the already not easy to maintain Rmd file even more unhandy.
Then I considered making logical variables from the variables I am using now, but that again made the whole code more complicated (at least in my eyes) and I refrained from it.
I guess I go with whats in now. Thanks for the feedback anyway!
Co-authored-by: Daniel Lundin <erik.rikard.daniel@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ironing out issues in the pipeline summary report, custom taxonomic databases weren't properly reported (NA).
Additionally moved a code snippet from
ampliseq.nfto a subworkflow, a purely cosmetic change.PR checklist
nf-core lint).nextflow run . -profile test,docker --outdir <OUTDIR>).docs/usage.mdis updated.docs/output.mdis updated.CHANGELOG.mdis updated.README.mdis updated (including new tool citations and authors/contributors).