Add coverm/contig to nf-core/modules

[vinisalazar]

Add CoverM to calculate read alignment metrics for contig/bin alignments (#587)

• Run nf-core modules install coverm/contig
• Create new module directory and modify modules.json

PR checklist

• 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 pipeline conventions in the contribution docs
• If necessary, also make a PR on the nf-core/mag branch on the nf-core/test-datasets repository.
• Make sure your code lints (nf-core pipelines lint).
• Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
• Check for unexpected warnings in debug mode (nextflow run . -profile debug,test,docker --outdir <OUTDIR>).
• Usage Documentation in docs/usage.md is updated.
• Output Documentation in docs/output.md is updated.
• CHANGELOG.md is updated.
• README.md is updated (including new tool citations and authors/contributors).

[vinisalazar]

Converting this to draft until I add the module logic to the subworkflows, will ping when done.

[vinisalazar]

Update to changes:

• in bin/get_mag_depths.py, add option to open gzip and non-gzip files
• contig depth calculation by default is now done with COVERM_CONTIG (METABAT2 is still an option)
• on mag_depths_convert/main.nf, define function to remove gz extension
• Add new params to nextflow.config with coverm options
• Update nextflow_schema.json with relevant new params
• On subworkflows/local/binning/main.nf:
  • Add checks to make sure CoverM mapper is bowtie2 (otherwise doesn't generate BAM files required for binning)
  • Enable both METABAT2 and COVERM as contig depth calculators
    • Same is done on subworkflows/local/binning_preparation for SR and LR
  • Rename output channel from metabat2depths to contig_depths

[vinisalazar]

This one is good to go but will require merging nf-core/modules#10588 for the lint checks to pass.

[vinisalazar]

PS all code generated by @claude was reviewed

[vinisalazar]

Making this into draft again until the upstream update to the coverm module is merged.

[jfy133]

Hi @vinisalazar, sorry for the delay on the review, I had a busy week after all.

Regarding the nf-test errors: this was due to a malformation in the snapshot when editing it manually (when a version has 3 components X.Y.Z, it has to be in quotes, so "coverm": 0.7.0 should be "coverm": "0.7.0").

Regarding the implementation, this looks nice! but I still feel that this needs some deeper integration to really take advantage of coverm. In general, I have the following ideas about how the process should look:

1. coverm contig should replace the manual alignment step in binning_preparation (except when using bowtie2, obviously), aligning the reads against the contigs and exporting both the depth file and the BAM files. So, coverm contig should live in binning_preparation, not in the binning subworkflow. For this:
   • Instead of coverage_mapper, I think it should be called shortread_coverage_mapper to be more explicit, and the options should only include short-read mappers (minimap2-sr, bwa-mem, bwa-mem2, strobealign).
   • For long reads, coverm should also replace the manual mapping in binning_preparation_longread, choosing the appropriate mapper (minimap2-ont/minimap2-pb/minimap2-hifi) based on the long-read platform from the samplesheet metadata.
   • Right now the coverm/contig module doesn't emit the cached BAM files as outputs, so this will need a module update. I can do that tomorrow!
2. The method for coverm contig should remain metabat, as this output is what the binners (MetaBAT2, MaxBin2, MetaBinner) expect.
3. For calculating per-bin coverage downstream, we should replace the get_mag_depths.py script with coverm genome, using the BAMs from the previous step (coverm contig or bowtie2) as input plus the bin FASTAs. We could also expose --methods as a pipeline parameter so users can choose which metric(s) to use (trimmed_mean, mean, covered_fraction, relative_abundance, etc). The question here is: if a user chooses multiple methods, which one should we report in bin_summary.tsv? Perhaps only the first one, leave the full coverm output in its TSV?

We can definitely leave step 3 for another PR and focus on the coverm contig integration in this one to keep things manageable.

What do you think about this @jfy133, @prototaxites?

Brief look looks fine to me, only 3 needs more careful so I agree to push to another PR!

[prototaxites]

Yes, agreed, your plan sounds good to me!

[vinisalazar]

Thanks @dialvarezs @jfy133 @prototaxites for the comments. Hope everyone could get some days off for Easter. I will be picking this up on Monday to implement points 1 and 2 suggested by Diego.

[vinisalazar]

@dialvarezs I'm having some difficult interpreting the failing checks here, could I please request another review?

Thanks

[jfy133]

@dialvarezs I'm having some difficult interpreting the failing checks here, could I please request another review?

Thanks

The rorr is:

The current character read is ']' with an int value of 93
Unable to determine the current character, it is not a string, number, array, or object
line number 224
index number 13623
            ],
............^

You're somehow missing a , in the .snap files most likely - or rather you've got an extra trailing one

[jfy133]

Let's try this

[jfy133]

I suspect it is this, as I learnt the hard way it's best not to edit snapshot files directly for the same reason 😆

[jfy133]

I think I fixed the original errors but appears tehre are others now

[dialvarezs]

Hi @vinisalazar! As @jfy133 mentioned, it’s generally a good idea to update snapshots manually, since automatic updates can result in malformed JSON or subtle issues things nf-test may not like (e.g. differences in ordering).

I’ve now regenerated all the snapshots. If you’d like a full review, I can most likely do it over the weekend.

[dialvarezs]

For the meantime, it seems that something is not working on long read data. Many processes disspaeared from the snapshot, so I suppose that mag is finishing early now.
Can you give it a look?

[vinisalazar]

Understood, apologies for that. My laptop can't handle the tests so I was struggling to update the snapshots programmatically. Over the weekend investigate the issue on the long reads data on a bigger machine that can actually run the tests.

[jfy133]

Hi @vinisalazar! As @jfy133 mentioned, it’s generally a good idea to update snapshots manually, since automatic updates can result in malformed JSON or subtle issues things nf-test may not like (e.g. differences in ordering).

I’ve now regenerated all the snapshots. If you’d like a full review, I can most likely do it over the weekend.

Uhhh don't you mean the opposite? Manual bad, automatic,good?

[dialvarezs]

Hi @vinisalazar! As @jfy133 mentioned, it’s generally a good idea to update snapshots manually, since automatic updates can result in malformed JSON or subtle issues things nf-test may not like (e.g. differences in ordering).
I’ve now regenerated all the snapshots. If you’d like a full review, I can most likely do it over the weekend.

Uhhh don't you mean the opposite? Manual bad, automatic,good?

Oops, my bad. Not the smartest idea to give recommendations when I’m still half asleep 😅.