-
Notifications
You must be signed in to change notification settings - Fork 141
BIgMAG compatibility #861
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
base: dev
Are you sure you want to change the base?
BIgMAG compatibility #861
Conversation
@harper357 I think your PR #842 might be relevant for this No pressure (just trying to work out priorities), but I was wondering how far off you think your PR would be? If so we could maybe get that in first and it might make @jeffe107 's life easier? What do you think? If you think it'll take you a while still then we can just proceed with Jeferyd's system here and later on once your PR is merged in change the logic @jfy133 I am hoping to have it done this week as long as life stuff doesn't eat up all my time. I will try to look at BIgMAG to make sure I am not doing anything that will make it harder to add it. |
@nf-core-bot fix linting |
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.
First pass for checking adherence to nf-core guidelines/the mag conventions (i.e., without testing), but it looks really good for a first contribution, thank you @jeffe107 👏
@@ -0,0 +1,55 @@ | |||
process CONCAT_BIGMAG { |
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.
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.
Yes and no. It does exactly the same, the difference relies on that modules/nf-core/csvtk/concat
takes binqc_tool
as parameter to generate the output file. If I call the same module on bigmag subworkflow, it would overwrite the file generated during BIN_QC
subworkflow. As a result, I created CONCAT_BIGMAG
to rename the binqc_tool
and create the file for the tool that bigmag subworkflow
is executing.
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.
Where would it re-write the file? You mean in --outdir
? If that's the case we can just change the resulting file's name using publishDir
for an aliased CSVTK module in modules.config
:)
Unless I misunderstand?
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.
Yes, you are right, it would overwrite the file in --outdir
. So, great, I agree with the solution you propose.
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.
For other reviewers: this is generally redundant, but we will wait for @harper357 's PR before we simplify this subworkflow
Co-authored-by: James A. Fellows Yates <[email protected]>
Co-authored-by: James A. Fellows Yates <[email protected]>
Co-authored-by: James A. Fellows Yates <[email protected]>
Co-authored-by: James A. Fellows Yates <[email protected]>
Co-authored-by: James A. Fellows Yates <[email protected]>
Hi all, a couple of initial thoughts on a quick passs-through:
I would prefer if we just transparently refused to run the subworkflow if the required tools are not enabled. We've gone to some effort to have most of the tools be configurable with flags, and I wouldn't want to pre-empt the user's choices in this matter. We should just print a warning if the BigMag subworkflow can't run.
Unless I'm missing something, as James says, you can definitely just re-use CSVTK_CONCAT. You just import it:
And then you can reference |
OK I'm back from the nf-core core team retreat and a conference, sorry for the delay and thanks for the patience @jeffe107 ! Looking through this PR again, I do think we should work off of @harper357 's PR. I've offered to take that over so we can get that in and have something easier for you to work with. So I'll focus on that next week and then we can re-jig this PR based on those changes :) And I agree with @prototaxites that I (once Greg's PR is in), that we should just error on pipeline initialisation if BIgMAG is requested but the necessary QC tools are not activated. |
Hello,
I apologize for creating a new PR, I couldn't force the previous one to take the changes from dev branch. I include again my message, and in the next comment I will post your comments from the previous PR.
According to issue [#859 ] to produce the file that is used as input for BIgMAG I have included the following:
I didn't want to mess with the current state of the pipeline, so I included a subworkflow that handles the execution in this manner:
PR checklist
nf-core pipelines lint
).nextflow run . -profile test,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).