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

Added 2FAST2Q module #7318

Open
wants to merge 47 commits into
base: master
Choose a base branch
from
Open

Added 2FAST2Q module #7318

wants to merge 47 commits into from

Conversation

afombravo
Copy link

No description provided.

Copy link
Contributor

@SPPearce SPPearce left a comment

Choose a reason for hiding this comment

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

Is there a reason it isn't called 2fastq? Did something fall over because of the number at the start?

modules/nf-core/fast2q/tests/test.nf Outdated Show resolved Hide resolved
@afombravo
Copy link
Author

afombravo commented Jan 16, 2025

The reason I am "filling" it as fast2q is because the PyPI module is called fast2q (numbers at the start are a no go naming wise). The same for nextflow processes I find out? So, despite the tool being published as 2FAST2Q, internally I am keeping it as fast2q, in line with nomenclature.

@SPPearce
Copy link
Contributor

The reason the I am "filling" it as fast2q is because the PyPI module is called fast2q (numbers at the start are a no go naming wise), the same for nextflow processes i find out? So, despite the tool being published as 2FAST2Q, internaly I am keeping it as fast2q in line with numenclature.

Ok, that's fine, if it can't be 2FAST2Q then FAST2Q sounds like the best option.

@afombravo
Copy link
Author

This is apparently failing on the tests. @SPPearce you are more experienced. Could you provide some insight to what I need to change, please? Cheers!

modules/nf-core/fast2q/tests/main.nf.test Outdated Show resolved Hide resolved
modules/nf-core/fast2q/tests/main.nf.test Show resolved Hide resolved
modules/nf-core/fast2q/tests/main.nf.test Outdated Show resolved Hide resolved
afombravo and others added 15 commits January 19, 2025 20:22
…just as a way to check if it is working. The test data I was using from NF-core has the wrong input file format.
…just as a way to check if it is working. The test data I was using from NF-core has the wrong input file format.
…just as a way to check if it is working. The test data I was using from NF-core has the wrong input file format.
…just as a way to check if it is working. The test data I was using from NF-core has the wrong input file format.
@afombravo
Copy link
Author

@SPPearce The test errors I am getting now are due to some issue loading the right data. I am trying to use 2 files from here: https://github.com/nf-core/test-datasets/tree/crisprseq/testdata but the errors I am getting from the 2FAST2Q are related with the the files not being correct (either the wrong extention, or the wrong format). I know the files are right and the programs works with them "outside" nextflow, so, could it be some issue loading the right data from modules_testdata_base_path on nextflow.config?

@SPPearce
Copy link
Contributor

You shouldn't be trying to set modules_testdata_base_path here, that is globally set.
Ideally, you'd copy any required data into the modules branch on the test-datasets repository.
Currently, I think you are pointing at a github webpage, you'd need to change it to the raw data link on github:
https://raw.githubusercontent.com/nf-core/test-datasets/refs/heads/crisprseq/testdata/SRR8983579.small.fastq.gz.
However, I would suggest that you just use some of the already existing data in the test-datasets repository; it doesn't necessarily have to be particularly meaningful the output.

modules/nf-core/fast2q/main.nf Outdated Show resolved Hide resolved
modules/nf-core/fast2q/main.nf Outdated Show resolved Hide resolved
modules/nf-core/fast2q/main.nf Outdated Show resolved Hide resolved
@afombravo afombravo requested a review from SPPearce January 20, 2025 16:26
@afombravo
Copy link
Author

@SPPearce seems to be ready for one last review and approval. Sorry for having spammed too much!

modules/nf-core/fast2q/tests/nextflow.config Show resolved Hide resolved
modules/nf-core/fast2q/tests/main.nf.test Outdated Show resolved Hide resolved
modules/nf-core/fast2q/tests/main.nf.test Outdated Show resolved Hide resolved
modules/nf-core/fast2q/tests/main.nf.test Show resolved Hide resolved
modules/nf-core/fast2q/tests/main.nf.test Show resolved Hide resolved
modules/nf-core/fast2q/tests/main.nf.test Show resolved Hide resolved
modules/nf-core/fast2q/tests/main.nf.test Outdated Show resolved Hide resolved
@afombravo
Copy link
Author

@SPPearce I made all tests snapshot comparisons and they are working locally. Everything seems ok. Any more comments?

@afombravo afombravo requested a review from SPPearce January 23, 2025 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants