search

nf-core / modules

Repository to host tool-specific module files for the Nextflow DSL2 community!
https://nf-co.re/modules
MIT License
283 stars 719 forks source link

Update hifiasm inputs #6799

Closed fellen31 closed 3 weeks ago

fellen31 commented 4 weeks ago

Is your feature request related to a problem? Please describe

The paternal and maternal kmer inputs (as well as the HiC inputs) are specific to the reads, and I think they should be input together, i.e.:

tuple val(meta), path(reads), path(paternal_kmer_dump), path(maternal_kmer_dump), path(hic_read1), path(hic_read2)

Otherwise, I think there's no way to ensure that the reads and kmers that belong together are input to the process together, so Sample1 could get Sample2's parental kmers.

Describe the solution you'd like

No response

Describe alternatives you've considered

No response

Additional context

No response

fellen31 commented 4 weeks ago

Otherwise, I think there's no way to ensure that the reads and kmers that belong together are input to the process together, so Sample1 could get Sample2's parental kmers.

We usually use multiMap for this.

I'll continue to ask here if that's ok @mahesh-panchal. I haven't really needed to use multiMap.

Many modules do tuple val(meta), path(bam), path(bai), I guess instead of using multiMap with:

tuple val(meta), path(bam)
tuple val(meta), path(bai)

Do you prefer the currently implemented solution, instead the one I proposed in this issue?

Thanks!

mahesh-panchal commented 4 weeks ago

I'm aware they're usually bound together in a tuple. I do prefer that too, but then I would prefer the modules become usable more like functions like demo'ed in https://github.com/nf-core/fetchngs/pull/309/files.

I was merely saying there is a way to synchronize separate channels.

fellen31 commented 4 weeks ago

Ok, thanks for the clarification and the tip on multiMap for synchronizing channels!

Schmytzi commented 3 weeks ago

I was considering updating the HiFiAsm module to 0.20 anyway (also bc @fellen31 requested that). I'd prefer having the paternal data in a tuple together with the reads so I would implement that change together with the update. However, from the way this discussion is going, it seems to me that the process definition should stay as-is. Is that correct or am I "free" to change it?

mahesh-panchal commented 3 weeks ago

You can change it. Please ping me for review and I can ping some others who use this module so we're aware of the update. There will be a discussion then if it's problematic.