-
Notifications
You must be signed in to change notification settings - Fork 35
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
Refactor #574
Refactor #574
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.
Looks good, just two small comments.
|
||
- name: Run the downloaded pipeline (stub) | ||
id: stub_run_pipeline | ||
continue-on-error: true | ||
env: | ||
NXF_SINGULARITY_CACHEDIR: ./ | ||
NXF_SINGULARITY_HOME_MOUNT: true | ||
run: nextflow run ./${{ env.REPOTITLE_LOWERCASE }}/$( sed 's/\W/_/g' <<< ${{ env.REPO_BRANCH }}) -stub -profile test,singularity --outdir ./results |
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.
So now neither the stub run nor the regular run of the downloaded pipeline is tested? As a user of an offline cluster, it's very nice to know that nf-core download
works smoothly. But if there's no space maybe there's nothing to do.
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.
Yeah, that's right. The downloaded pipeline is not tested anymore. I can make sure that the downloaded pipeline works locally before a release, but the github runners just don't have enough space for both the downloaded containers and the tests :(
CHANGELOG.md
Outdated
| --------------- | ------------------------ | | ||
| | mbuffer_mem | | ||
| | samtools_sort_threads | | ||
| | skip_repeat_analysis | |
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 skip_repeat_calling
would be more inline with the subworkflow name, the current output and usage docs ("Variant calling - repeat expansions"), and the parameters below.
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.
hmmm.. good point. The workflow includes stranger which, as you know already, is used to annotate STRs. So it does perform more than repeat calling. Perhaps I need to change the name of the subworkflow, and its references in the pipeline 😅
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 agree skip_repeat_analysis is more inline with what it actually does.
What about splitting the subworkflow into call and annotate, like you do for SNVs and SVs? Then we could use the same annotation subworkflow in both raredisease and Nallo, and you can have skip_repeat_calling
and skip_repeat_annotation
.
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.
Great minds think like 😆
That's what I am actually doing right now :D Its much easier, and like you said, it is in line with what we are doing for other variant types.
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.
Very nice! ⭐
PR checklist
nf-core lint
).nextflow run . -profile test,docker --outdir <OUTDIR>
).nextflow run . -profile test_one_sample,docker --outdir <OUTDIR>
).nextflow run . -profile debug,test,docker --outdir <OUTDIR>
).docs/usage.md
is updated.docs/output.md
is updated.CHANGELOG.md
is updated.README.md
is updated (including new tool citations and authors/contributors).