Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Iqtree version bump and output fix #5618

Merged
merged 19 commits into from
May 29, 2024
Merged

Iqtree version bump and output fix #5618

merged 19 commits into from
May 29, 2024

Conversation

tstoeriko
Copy link
Contributor

@tstoeriko tstoeriko commented May 16, 2024 •
edited

PR checklist

Closes #5607

  • This comment contains a description of changes (with reason).
  • bump version from 2.3.0 to 2.3.4
  • include all optional input and output files (as documented in the IQ-Tree command reference)
  • add tests for multiple different input combinations/modes
  • If you've fixed a bug or added code that should be tested, add tests!
  • Follow the naming conventions.
  • Follow the input/output options guidelines.
  • 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

@maxulysse
Copy link
Member

looking good, but I feel you added some optional files, is there any possibility to test for it too?

@tstoeriko
Copy link
Contributor Author

looking good, but I feel you added some optional files, is there any possibility to test for it too?

Yes, the module was basically missing all optional output files, likely because there is no centralized documentation of the possible output files. I'll have a look at the tests again and try to include as many of the optional files as possible.

@famosab
Copy link
Contributor

famosab commented May 16, 2024

You could also add a stub test while you are doing that :)

@tstoeriko
Copy link
Contributor Author

You could also add a stub test while you are doing that :)

Will make sure to add stub tests for any additional tests I implement. 👍

@tstoeriko
Tweak tests
ac8532d 
- remove basic test (all outputs covered elsewhere)
- skip model and tree search wherever possible
- use same data for all tests
@tstoeriko
Tweak tests
c3ae781 
- skip model and tree search wherever possible
- use smallest alignment when no tree is needed
@tstoeriko
Copy link
Contributor Author

This module has now been majorly reworked due to missing inputs and outputs and addition of new tests. I would be grateful for some more feedback.

Major changes:

  • Included all optional input and output files. Though there is no centralized documentation of input and output file options, so I cannot guarantee I've caught them all.
  • Removed one of the previous inputs (fconst) which is not a file and should be fine to supply via ext.args.
  • Made the main alignment input file optional as there are modes which do not require an alignment.
  • Wrote tests for nearly all output files.

There are a couple of things I am struggling with:

  • The module now has 14 inputs (up from 2), I am a bit worried about usability. Will users just have to adapt?
  • I've tried to include relevant file extensions in the meta.yml, but I think IQ-Tree itself doesn't actually check for them. Should I keep the patterns?
  • I'd love some feedback on the variable names I've chosen. I think with this huge number of inputs and outputs it is important to make them as intuitive as possible.
  • I've spent quite some time on setting up appropriate tests, but I am still missing tests for three specific outputs.
    The input data currently available do not work for these tests, they are likely not complex enough.
    I am personally not familiar enough with the corresponding modes to fully troubleshoot the input data requirements.
    I've tested some data locally, but have not been able to find a suitable small dataset to use.
    I'm happy to take some time in the future to find a good dataset, but maybe we could get this PR merged for now?

@tstoeriko tstoeriko marked this pull request as ready for review May 23, 2024 16:30
mahesh-panchal
mahesh-panchal reviewed May 23, 2024
View reviewed changes
Copy link
Member

@mahesh-panchal mahesh-panchal left a comment

Choose a reason for hiding this comment

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

Not really knowing anything about the tool, this is a really nice effort. There's a smattering of comments to address.

modules/nf-core/iqtree/main.nf Outdated Show resolved Hide resolved
modules/nf-core/iqtree/tests/iqtree_sup.config Outdated Show resolved Hide resolved
modules/nf-core/iqtree/tests/iqtree_basic.config Outdated Show resolved Hide resolved
@tstoeriko
Address comments
95886f0 
- remove obsolete config file
- remove non-required inputs from basic test (move to separate test)
- rename meta inputs
@tstoeriko tstoeriko marked this pull request as draft May 24, 2024 10:34
@SPPearce
Copy link
Contributor

You really don't have to test every possible input/output of the tool, so don't worry about missing a few files.
Are a lot of the input files mutually exclusive with each other?

tstoeriko and others added 3 commits May 24, 2024 12:47
@tstoeriko
Remove meta from most inputs
bad7326 
This commit removes the meta information from most inputs. A meta only
has to be provided with either alignment (meta) or tree file (meta2).
When meta is not supplied in the inputs, it gets assigned the value of
meta2.
@tstoeriko
Version bump 2.3.3 -> 2.3.4
51a98b6
@tstoeriko
Merge branch 'master' into iqtree-fix-output
88ad0f2
@tstoeriko
Copy link
Contributor Author

You really don't have to test every possible input/output of the tool, so don't worry about missing a few files. Are a lot of the input files mutually exclusive with each other?

I don't think they're mutually exclusive for the most part. Different combinations of input files will result in different modes being run though.

@tstoeriko tstoeriko marked this pull request as ready for review May 24, 2024 13:26
mahesh-panchal
mahesh-panchal approved these changes May 28, 2024
View reviewed changes
Copy link
Member

@mahesh-panchal mahesh-panchal left a comment

Choose a reason for hiding this comment

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

Another thing one could add to the tests is that certain channels should be empty.
Otherwise this is great work. Thank you.

modules/nf-core/iqtree/tests/main.nf.test Show resolved Hide resolved
@tstoeriko
Copy link
Contributor Author

Another thing one could add to the tests is that certain channels should be empty. Otherwise this is great work. Thank you.

Thanks for the suggestion and the thorough review in general, @mahesh-panchal ! I added some tests to confirm that certain optional output channels are empty. If there's nothing else missing, I'll go ahead and get this PR merged.

@mahesh-panchal
Copy link
Member

Sounds good. Great work.

@tstoeriko tstoeriko added this pull request to the merge queue May 29, 2024
Merged via the queue into nf-core:master with commit 6bcf632 May 29, 2024
12 checks passed
@tstoeriko tstoeriko deleted the iqtree-fix-output branch May 29, 2024 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mahesh-panchal mahesh-panchal mahesh-panchal approved these changes

Assignees

@tstoeriko tstoeriko

Labels
None yet
Projects
Hackathon: May 2024
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

iqtree: optional output files missing from module
5 participants
@tstoeriko @maxulysse @famosab @SPPearce @mahesh-panchal