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.

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

Adding TRUST4 module #4470

Merged
merged 14 commits into from
Mar 6, 2024
Merged

Adding TRUST4 module #4470

merged 14 commits into from
Mar 6, 2024

Conversation

Joaodemeirelles
Copy link
Contributor

@Joaodemeirelles Joaodemeirelles commented Nov 28, 2023

Draft for the TRUST4 module addition.

Since TRUST4 can be execute with single-end reads, paired-end reads and a bam file, I chose to handle this by testing if the channel exists (to differ between bam and reads), and the meta.single_end key to identify paired or single-end.

PR checklist

Closes #4307

  • 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.
  • Emit the versions.yml file.
  • 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:
    • 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

@Joaodemeirelles Joaodemeirelles linked an issue Nov 28, 2023 that may be closed by this pull request
4 tasks
@mapo9
Copy link
Contributor

mapo9 commented Dec 15, 2023

Hi Joao @Joaodemeirelles,

thanks for your work in adding this module!
I am also very interested to have this and would have some time now to work on it.

Can you tell me what still needs to be done?

Also, did you think about implementing tests yet?

@mapo9 mapo9 marked this pull request as ready for review January 23, 2024 06:54
@mapo9 mapo9 requested a review from a team as a code owner January 23, 2024 06:54
@mapo9 mapo9 requested a review from leoisl January 23, 2024 06:54
@mapo9
Copy link
Contributor

mapo9 commented Jan 23, 2024

Hi, I took this PR up again as I want to make use of the tool.

I experience some problems that I am unsure on how to deal with them.
First, nf-test fails as some files stay empty in the small test dataset. As linting failed due to md5sums for empty files, I removed the md5sums for these files. This made nf-test fail as the snapshots are now different due to the missing md5sums.

Also, singularity fails because there is no singularity container for the newest version of the tool and some output seems to have changed so the md5sums differ as well from the docker & conda run with the newest version.

Does anyone has an idea how to solve this?

@mapo9
Copy link
Contributor

mapo9 commented Jan 24, 2024

I was able to solve the problems above and think that the PR is now ready for review :)

@mapo9 mapo9 requested review from ggabernet and removed request for ggabernet January 24, 2024 10:37
@Joaodemeirelles
Copy link
Contributor Author

I was able to solve the problems above and think that the PR is now ready for review :)

Great! Sorry I could not help with the tests, thank you so much for the changes!

modules/nf-core/trust4/main.nf Outdated Show resolved Hide resolved
modules/nf-core/trust4/main.nf Outdated Show resolved Hide resolved
Comment on lines 12 to 13
path(fasta)
path(ref)
Copy link
Contributor

Choose a reason for hiding this comment

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

we need meta1 and meta2 here. What is the difference between fasta and ref, how about: vdj_reference?

Copy link
Contributor

Choose a reason for hiding this comment

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

ref is an additional reference required when you dont start from bam but fastq
fasta is what they call their reference basically.
I wanted to stay in line with their naming here, but am also fine with renaming them.

Copy link
Member

@ggabernet ggabernet left a 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 now! There's only the trinity pytest that I'm not sure they should be added here

@mapo9 mapo9 added this pull request to the merge queue Mar 6, 2024
Merged via the queue into master with commit dd1b3c4 Mar 6, 2024
11 checks passed
jch-13 pushed a commit to jch-13/modules that referenced this pull request Mar 19, 2024
* Adding TRUST4 module

* remove old pytest version

* trust4

* trust4

* linting

* modules/nf-core/trust4/environment.yml

* Update modules/nf-core/trust4/main.nf

Co-authored-by: Friederike Hanssen <[email protected]>

* review comments applied

---------

Co-authored-by: mapo9 <[email protected]>
Co-authored-by: Mark Polster <[email protected]>
Co-authored-by: Gisela Gabernet <[email protected]>
Co-authored-by: Friederike Hanssen <[email protected]>
jennylsmith pushed a commit to RSC-RP/modules that referenced this pull request Mar 20, 2024
* Adding TRUST4 module

* remove old pytest version

* trust4

* trust4

* linting

* modules/nf-core/trust4/environment.yml

* Update modules/nf-core/trust4/main.nf

Co-authored-by: Friederike Hanssen <[email protected]>

* review comments applied

---------

Co-authored-by: mapo9 <[email protected]>
Co-authored-by: Mark Polster <[email protected]>
Co-authored-by: Gisela Gabernet <[email protected]>
Co-authored-by: Friederike Hanssen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

new module: trust4
4 participants