Skip to content

New treerecs#10590

Open
takiifujita-ui wants to merge 16 commits intomasterfrom
new_treerecs
Open

New treerecs#10590
takiifujita-ui wants to merge 16 commits intomasterfrom
new_treerecs

Conversation

@takiifujita-ui
Copy link
Contributor

PR for the first time.

PR checklist

Closes #XXX

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the module conventions in the contribution docs
  • If necessary, include test data in your PR.
  • Remove all TODO statements.
  • Broadcast software version numbers to topic: versions - See version_topics
  • Follow the naming conventions.
  • Follow the parameters requirements.
  • Follow the input/output options guidelines.
  • Add a resource label
  • Use BioConda and BioContainers if possible to fulfil software requirements.
  • Ensure that the test works with either Docker / Singularity. Conda CI tests can be quite flaky:
    • For modules:
      • nf-core modules test <MODULE> --profile docker
      • nf-core modules test <MODULE> --profile singularity
      • nf-core modules test <MODULE> --profile conda
    • For subworkflows:
      • nf-core subworkflows test <SUBWORKFLOW> --profile docker
      • nf-core subworkflows test <SUBWORKFLOW> --profile singularity
      • nf-core subworkflows test <SUBWORKFLOW> --profile conda

Copy link
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, just smallish things here and there!

'biocontainers/treerecs:1.2--h9f5acd7_3' }"

input:
tuple val(meta), path(species_tree), path(gene_trees)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the gene and species trees directly linked to each other? If not they should be separated into distinct channels 1

Also I thin you're missing the optional -S map file - which needs to be supported 2

Footnotes

  1. https://docs-v2--nf-core-docs.netlify.app/docs/specifications/components/modules/input-output-options#required-path-channel-inputs

  2. https://docs-v2--nf-core-docs.netlify.app/docs/specifications/components/modules/general#required-and-optional-input-files


output:

tuple val(meta), path("*.nwk"), emit: corrected_trees
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ther are also -fevent files

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jfy133 Hi James, thanks for the comments! for this section, since user is able to specify output format, should these have "optional:true" and skip the "touch" on lines 40 on? I


"""
echo $args
touch ${prefix}.nwk
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When adding support for the other output files, make sure to include them here

id:'test' ]
- species_tree:
type: file
description: Species tree file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +18 to +19
file('https://gitlab.inria.fr/Phylophile/Treerecs/-/raw/master/examples/polytomy_example/speciesTree.txt?ref_type=heads', checkIfExists: true),
file('https://gitlab.inria.fr/Phylophile/Treerecs/-/raw/master/examples/polytomy_example/geneTree.txt?ref_type=heads', checkIfExists: true),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These neeed to be copied over to nf-core/testdatasets, so we can keep 'control' over them and are not deleted by or moved by someone else 1

Footnotes

  1. https://docs-v2--nf-core-docs.netlify.app/docs/specifications/components/modules/testing#input-data


then {
assert process.success
assert process.out.corrected_trees.size() == 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there no string you can check inside?

"""
input[0] = [
[ id:'test' ],
file('https://gitlab.inria.fr/Phylophile/Treerecs/-/raw/master/examples/polytomy_example/speciesTree.txt?ref_type=heads', checkIfExists: true),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

[
"TREERECS",
"treerecs",
"Treerecs (1.2), Inria - Beagle\nTreerecs 1.2"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

then {
assert process.success
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please wrap all of these 3 inside the same assertAll() 1. You can use other modules as examples

Footnotes

  1. https://docs-v2--nf-core-docs.netlify.app/docs/specifications/components/modules/testing#assertall

@jfy133 jfy133 mentioned this pull request Mar 12, 2026
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants