-
Notifications
You must be signed in to change notification settings - Fork 753
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
New module: fastme #4920
New module: fastme #4920
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 to me, just needs some of the optional outputs of fastme.
Nicely done!
modules/nf-core/fastme/main.nf
Outdated
|
||
input: | ||
path infile | ||
path topo |
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.
Thinks this is referring to an initial tree topology to use? Might be nice to give it a more descriptive name, something like initial_tree
or the likes.
|
||
output: | ||
path "*.nwk" , emit: nwk | ||
path "*_stat.txt" , emit: stats |
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 it would be good to add optional output channels at least for the more commonly used optional outputs, in particular the bootstrap supports.
modules/nf-core/fastme/meta.yml
Outdated
keywords: | ||
- phylogenetics | ||
- newick | ||
- minimum evolution |
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 keywords should have no spaces, could just be replaced with an underscore.
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.
Implementation of the optional outputs looks good, it should just be included into the stubs.
Also I think the test covering it looks a little weird with the call to snapshot that isn't doing anything.
|
||
} | ||
|
||
test("setoxin - phylip - with_optional_output") { |
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.
Just a small thing, I would rename this to bootstrap since that is what it's testing
{ assert snapshot(process.out.nwk) }, | ||
{ assert snapshot(process.out.matrix) }, | ||
{ assert snapshot(process.out.bootstrap) } |
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.
As I understand, the bootstrap result is not deterministic so should not be snapshotted, and without calling .match() it shouldn't even generate a snapshot I don't think.
I suggest just checking for the files manually, or snapshot-matching the list of files in the output dir.
- matrix: | ||
type: file | ||
description: Optional; the distance matrix in Phylip matrix format; it is generated if the -O option is passed in ext.args, although the provided file name will be overwritten | ||
pattern: "*.matrix.phy" | ||
- bootstrap: | ||
type: file | ||
description: A file containing all bootstrap trees in Newick format; it is generated if the -B option is passed in ext.args (and bootstrap is used), although the provided file name will be overwritten | ||
pattern: "*.bootstrap" |
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.
Nice!
modules/nf-core/fastme/main.nf
Outdated
def args = task.ext.args ?: '' | ||
def prefix = task.ext.prefix ?: infile | ||
""" | ||
touch ${prefix}.nwk | ||
touch ${prefix}_stat.txt |
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.
As a pipeline may rely on the optional output of -B, the stub should also generate these if the argument is passed.
Looks good, merging! |
* Added main, started creating tests * Fixed tests * Fixed tests again * Fixed tests, completed meta.yml * Minor lint issue fix * Fix typo * Renamed initial tree argument, added optional outputs and the associated test * Removed a space in meta.yml * Added optional outputs to stub, changed bootstrap test --------- Co-authored-by: Leon Rauschning <[email protected]>
* Added main, started creating tests * Fixed tests * Fixed tests again * Fixed tests, completed meta.yml * Minor lint issue fix * Fix typo * Renamed initial tree argument, added optional outputs and the associated test * Removed a space in meta.yml * Added optional outputs to stub, changed bootstrap test --------- Co-authored-by: Leon Rauschning <[email protected]>
PR checklist
Closes #4889
versions.yml
file.label
PROFILE=docker pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware
PROFILE=singularity pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware
PROFILE=conda pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware
Note: Conda test was not run locally due to a conda issue seemingly independent of the module (exit code 143, terminated by the system).