Conversation
| // WARN: Version information not provided by tool on CLI. Please update this string when bumping container versions. | ||
| tuple val("${task.process}"), val('bigslice'), val("2.0.2"), topic: versions, emit: versions_bigslice |
There was a problem hiding this comment.
working on it rn sorry
jfy133
left a comment
jfy133
left a comment
There was a problem hiding this comment.
@vagkaratzas any thoughts as well?
modules/nf-core/bigslice/main.nf
Outdated
| def export_tsv_cmd = args2 ? """ | ||
| bigslice \\ | ||
| --export-tsv ${prefix}/result/tsv_export \\ | ||
| --program_db_folder ${hmmdb} \\ | ||
| ${prefix} | ||
| """ : '' |
There was a problem hiding this comment.
I do not understand this implementation, what is args2 trying to do here to activate the execution?
Shouldn't you just have a boolean input value channel that, if true, just injects --export-tsv in to the main command? Why the whole extra command?
There was a problem hiding this comment.
Agreed, ext.args can have more than one param - value pair inside. You don't need a new args* value for each param possible. --export-tsv can be passed and checked through ext.args
There was a problem hiding this comment.
--export-tsv is a separate BiG-SLiCE subcommand that must be called as a second bigslice invocation after clustering completes, so it cannot simply be appended to the main command. Two approaches exist: using ext.args2 to explicitly separate the two commands (clear intent, but adds a non-standard extra args variable), or detecting --export-tsv inside ext.args, stripping it from the main command and running it as a post-step (current approach), which keeps a single ext.args entry point. If you have any other suggestions please tell :')
There was a problem hiding this comment.
OK then yeah I would go for a boolean input channel, and inject the second command (with $args2) if requested.
For variable-based command injection, keep the command on one line rather than across multiple though
There was a problem hiding this comment.
Definitely add a new nf-test case testing this new funcitonality.
I am confused as to which method to choose, because I can see the --export-tsv being used twice; both here --export-tsv ${prefix}/result/tsv_export \\ and in the ${export_tsv_cmd} command afterwards.
I'd definitely skip args2, and parse ext.args for finding it though.
There was a problem hiding this comment.
Given it fundamentally changes the command, I feel input channel is clearer/more explicit
|
no more results folder it will be sample/(bigslice output) fully working parameters with catch for errors if you have any suggestions please tell |
| // Flatten the GBK directory into a list of individual GBK files with meta | ||
| input[0] = UNTAR_GBK.out.untar.map { meta, dir -> | ||
| def gbk_files = [] | ||
| dir.eachFileRecurse { if (it.name.endsWith('.gbk')) gbk_files << it } |
There was a problem hiding this comment.
modules/nf-core/bigslice/main.nf
Outdated
| mv ${prefix}/result/data.db ${prefix}/data.db | ||
| mv ${prefix}/result/tmp ${prefix}/tmp | ||
| rm -rf ${prefix}/result |
There was a problem hiding this comment.
I don't think it's necessary to do these move operations (and actually it's not recommened apparently), youc an just emit prefix as is
There was a problem hiding this comment.
I had issues before getting to the files afterwards when I did not do the mv (where are the recommendations on this?)
famosab
left a comment
famosab
left a comment
There was a problem hiding this comment.
I added a few comments to your PR.
| { assert resultDir.isDirectory() }, | ||
| { assert file("${resultDir}/data.db").exists() }, | ||
| { assert resultDir.list().any { it.endsWith('.fa') || new File("${resultDir}/tmp").exists() } }, | ||
| { assert snapshot( | ||
| process.out.findAll { key, val -> key.startsWith("versions")} | ||
| ).match() } |
There was a problem hiding this comment.
Can you add other files to this snapshot as well? Ideally we want all outputs to be at least present by name in the snapshot.
|
@famosab |
|
Is there something else I can implement or do to this module ? I am planing to do something like antismash to automatically download the hmmdb without the user to manually download it. |
|
From my side we are good but I guess this should be @vagkaratzas deciding since he wrote the module at some point :) |
vagkaratzas
left a comment
vagkaratzas
left a comment
There was a problem hiding this comment.
Merge now, can always improve later.
6 participants
PR checklist
Closes #XXX
topic: versions- See version_topicslabelnf-core modules test <MODULE> --profile dockernf-core modules test <MODULE> --profile singularitynf-core modules test <MODULE> --profile condanf-core subworkflows test <SUBWORKFLOW> --profile dockernf-core subworkflows test <SUBWORKFLOW> --profile singularitynf-core subworkflows test <SUBWORKFLOW> --profile conda