Migrate CAT local modules to nf-core ones #799
Conversation
|
@nf-core-bot fix linting |
prototaxites
left a comment
There was a problem hiding this comment.
Hi @dialvarezs, I don’t have time to run any tests at the moment but wanted to say this looks really good - just one little comment so far. I’ll try and take a more in depth look later next week!
d4straub
left a comment
There was a problem hiding this comment.
I had a look and found it looks great I think! However, I had no time to run the pipeline, so I hesitate to give it an approval (and I am just getting used to mag again).
jfy133
left a comment
There was a problem hiding this comment.
Looking good again @dialvarezs !
- Can you double check there is no change to the output structure, and thus no changes needed there (given you've added CATPACK_BINS`, I wonder if we should describe that there
Otherwise I don't think there is any major blockers from me here, other than maybe the missing SUMMARISE for CATPACK_BINS
Once the changes here are made I would like to do my own quick test and then I think we are good o go
Very clean PR, thank you again!
Co-authored-by: James A. Fellows Yates <[email protected]>
|
@nf-core-bot fix linting |
jfy133
left a comment
There was a problem hiding this comment.
LGTM now @dialvarezs ! Thank you very much, I think this will be really helpful!
If he has time maybe @prototaxites can have another look through (as he's given quite a few comments before), and then merge.
Co-authored-by: James A. Fellows Yates <[email protected]>
prototaxites
left a comment
There was a problem hiding this comment.
Hi @dialvarezs, this looks great! Couple of small comments - the one about params passing is definitely optional and it might make sense to do as a single PR refactoring all pipeline inputs down the line.
| ======================================== | ||
| */ | ||
|
|
||
| if (params.cat_db) { |
There was a problem hiding this comment.
Future-proof thinking: Is it better to pass the DB into this subwf as a channel, rather than reading it direct from the params? In future, it looks like params will only be available in the entry workflow and need to be passed through as arguments.
There was a problem hiding this comment.
Yeah, I guess it makes more sense to update this across the entire pipeline in a single PR.
Could you open an issue to keep track of this change?
| ========================================= | ||
| */ | ||
|
|
||
| if (params.cat_classify_unbinned) { |
Co-authored-by: Jim Downie <[email protected]>
|
Thanks for the reviews, @jfy133, @prototaxites, @d4straub! |
Main changes:
Closes #611, #735.
PR checklist
nf-core pipelines lint).nextflow run . -profile test,docker --outdir <OUTDIR>).nextflow run . -profile debug,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).