Refactor code and configs #490
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
|
jemten
reviewed
Jan 30, 2024
| @@ -24,11 +24,12 @@ process { | |||
| } | |||
|
|
|||
| withName: '.*CALL_REPEAT_EXPANSIONS:EXPANSIONHUNTER' { | |||
| ext.args = { ("${meta.sex}" == 1) ? '--sex male' : '--sex female' } | |||
| ext.args = { ("${meta.sex}" == '1') ? '--sex male' : '--sex female' } | |||
There was a problem hiding this comment.
Was this broken? From the schema it looks like sex needs to be an integer and not a string.
raredisease/assets/schema_input.json
Line 42 in 2be203d
I guess that there is a risk that the old ("${meta.sex}" == 1) simply checks for the boolean. Anyway, can you confirm that this sets the gender correctly for male and female?
There was a problem hiding this comment.
Schema does expect the value to be an integer, but in meta everything is of string type.. I retrieved the type of the values in our meta and this is what it looks like..
[id:java.lang.String, sample:java.lang.String, lane:java.lang.String, sex:java.lang.String, phenotype:java.lang.String, paternal:java.lang.String, maternal:java.lang.String, case_id:java.lang.String, num_lanes:java.lang.String, read_group:java.lang.String, single_end:java.lang.Boolean]
There was a problem hiding this comment.
I see, didn't know that :)
In that case we should look into whether this needs changing this as well
Worked for my testing previously though
Also it might be worth looking into whether this function needs changing
raredisease/workflows/raredisease.nf
Line 745 in 2be203d
conf/modules/annotate_cadd.config
Outdated
| @@ -16,7 +16,7 @@ | |||
| // | |||
|
|
|||
| process { | |||
| if (params.cadd_resources != null) { | |||
| if (!params.cadd_resources != "null") { | |||
There was a problem hiding this comment.
is null a string here? Maybe we can do something like this if we just want to check if cadd_resources was given.
Suggested change
| if (!params.cadd_resources != "null") { | |
| if (params.cadd_resources) { |
Would this work?
There was a problem hiding this comment.
I doubt this will work because null here is a string and so the expression might not evaluate to false..
There was a problem hiding this comment.
Ah, OK. Struggling a bit to read the double negation here. Seems like we flipped the original logic of the statement 😅
There was a problem hiding this comment.
That conditional was not necessary at all so I have removed it.
jemten
approved these changes
Jan 31, 2024
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.
PR checklist
nf-core lint).nextflow run . -profile test,docker --outdir <OUTDIR>).nextflow run . -profile test_one_sample,docker --outdir <OUTDIR>).docs/usage.mdis updated.docs/output.mdis updated.CHANGELOG.mdis updated.README.mdis updated (including new tool citations and authors/contributors).