migrating flash to nf-test

[DavideBag]

PR checklist

• Migrating flash module to nf-test.
• If you've fixed a bug or added code that should be tested, add tests!
• Emit the versions.yml file.
• Follow the naming conventions.
• Follow the parameters requirements.
• Follow the input/output options guidelines.
• 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

[SPPearce]

I don't understand what the args2 is for here, when it is just next to args in the code

[SPPearce]

Does this module take fastq files in and produce different ones? If so, can you include a check that the output files have different names

[SPPearce]

Does this module take fastq files in and produce different ones? If so, can you include a check that the output files have different names

Or you could give them a suffix to make it clear they have been through this tool.

[lescai]

Hi @SPPearce and @DavideBag
I understand that preference is always not to hard-code any prefix / suffix, and leave this to the config.
however, as Simon highlights one should include an explicit check whenever there's the risk of an overlap in input/output names.
Best practice I'd suggest is the one adopted in the samtools module, since in sorting / slicing this is very common:
https://github.com/nf-core/modules/blob/master/modules/nf-core/samtools/sort/main.nf

[SPPearce]

There are definitely many fastq tools that don't follow that convention though

[DavideBag]

I have double checked the default output of the software which is as follows:

The default output of FLASH consists of the following files:

   - out.extendedFrags.fastq      The merged reads.
   - out.notCombined_1.fastq      Read 1 of mate pairs that were not merged.
   - out.notCombined_2.fastq      Read 2 of mate pairs that were not merged.
   - out.hist                     Numeric histogram of merged read lengths.
   - out.histogram                Visual histogram of merged read lengths.

There is no way to change the prefix added as "extendedFrags" or "notCombined".
We can introduce an input/output check as suggested, although there is little probability the input will contain those words, ending up in an overlap.

Most importantly, however, the current version of the module captures all outputs in a single channel, while they should be separated because one is the expected result (i.e. the merged reads = extendedFrags) and the other contains the not combined ones.
The potential issue here is that this change might be a breaking change for other pipelines, who can however patch the module if they wish to keep all reads in a single channel.
I have discussed this with @lescai but asking @maxulysse for advice on this, before adding the changes to this PR.

[SPPearce]

I'm happy to introduce breaking changes that make sense. One option is to rename the output channel, then it will definitely fail and force people to do something

…

On Mon, 29 Apr 2024, 10:27 DavideBag, ***@***.***> wrote: I have double checked the default output of the software which is as follows: The default output of FLASH consists of the following files: - out.extendedFrags.fastq The merged reads. - out.notCombined_1.fastq Read 1 of mate pairs that were not merged. - out.notCombined_2.fastq Read 2 of mate pairs that were not merged. - out.hist Numeric histogram of merged read lengths. - out.histogram Visual histogram of merged read lengths. There is no way to change the prefix added as "extendedFrags" or "notCombined". We can introduce an input/output check as suggested, although there is little probability the input will contain those words, ending up in an overlap. Most importantly, however, the current version of the module captures all outputs in a single channel, while they should be separated because one is the expected result (i.e. the merged reads = extendedFrags) and the other contains the not combined ones. The potential issue here is that this change might be a breaking change for other pipelines, who can however patch the module if they wish to keep all reads in a single channel. I have discussed this with @lescai <https://github.com/lescai> but asking @maxulysse <https://github.com/maxulysse> for advice on this, before adding the changes to this PR. — Reply to this email directly, view it on GitHub <#5470 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF55TWMTKK6GOWKUL4GPP3DY7YHBHAVCNFSM6AAAAABGCN2F5OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBSGI3DIMJUGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[lescai]

LGTM now, quite thorough and now more explicit with flash documentation.

[DavideBag]

Thank you to @SPPearce and @lescai for investing time in reviewing.