Skip to content

Add PyPOLCA polishing and use polished assemblies for long-read binning#1048

Open
Harshita-sriv wants to merge 15 commits into
nf-core:devfrom
Harshita-sriv:add-polishing-module
Open

Add PyPOLCA polishing and use polished assemblies for long-read binning#1048
Harshita-sriv wants to merge 15 commits into
nf-core:devfrom
Harshita-sriv:add-polishing-module

Conversation

@Harshita-sriv

Copy link
Copy Markdown

Summary

This PR adds a PyPOLCA polishing module and integrates polished assemblies into the long-read MAG workflow.

Changes

  • Added a new PyPOLCA module and conda environment.
  • Added publication of PyPOLCA outputs.
  • Added the run_pypolca parameter.
  • Modified the workflow so that long-read binning uses ch_polished_assemblies instead of raw ch_longread_assemblies.

Validation

Tested on one Nanopore metagenomic sample.

Verified that:

  • PyPOLCA successfully generated sample_polished.fasta.
  • LONGREAD_BINNING_PREPARATION:MINIMAP2_ASSEMBLY_INDEX used sample_polished.fasta.
  • Coverage estimation and downstream binning (MetaBAT2, MaxBin2, CONCOCT, COMEBin, MetaBinner, DAS Tool) executed after PyPOLCA completion.

This ensures MAG generation is performed using polished assemblies rather than raw Flye assemblies.

@dialvarezs

dialvarezs commented Jun 2, 2026

Copy link
Copy Markdown
Member

Hi @Harshita-sriv, thanks for this!
For the meantime, can you join the nf-core org? That way you will able to run the ci without explicit approval.
https://nf-co.re/join

@dialvarezs dialvarezs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In general I think it looks right, but needs to migrate to the nf-core module, and of course, pass the ci.

Comment thread modules/local/pypolca/main.nf Outdated
Comment thread workflows/mag.nf Outdated
Comment thread workflows/mag.nf
@Harshita-sriv

Copy link
Copy Markdown
Author

Thanks for the review. I investigated the CI failures and found that the current implementation does not define ch_polished_assemblies when run_pypolca is not enabled, which causes the test workflows to fail. I'll address this together with the requested migration to the official nf-core PyPOLCA module, removal of debug statements, and formatting fixes

@Harshita-sriv Harshita-sriv requested a review from dialvarezs June 3, 2026 09:58
@dialvarezs

Copy link
Copy Markdown
Member

Hi @Harshita-sriv! I think this looks fine, but it would be ideal to wire it up in some test. I'll think what would be the best option here and I'll come back to you.
For the meantime, can you update the changelog?
Thanks!

@d4straub d4straub left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the addition, I think thats absolutely desired!
Looks good but I have some concerns with multiple samples.

Comment thread nextflow_schema.json Outdated
Comment thread workflows/mag.nf Outdated
Comment thread conf/modules.config Outdated
@Harshita-sriv

Copy link
Copy Markdown
Author

I noticed that the remaining lint failure is due to modifications in modules/nf-core/pypolca/run, which no longer matches the upstream module.

The changes were introduced because PyPOLCA was failing during testing, and with the modifications the pipeline completes successfully on the test dataset.

Would it be acceptable to keep these modifications to the PyPOLCA module, or would you prefer that the implementation uses the upstream module unchanged and handles the required logic elsewhere?

@d4straub

d4straub commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

Would it be acceptable to keep these modifications to the PyPOLCA module, or would you prefer that the implementation uses the upstream module unchanged and handles the required logic elsewhere?

Some of the changes are not acceptable: the output block and the versioning must be kept as it was originally. Additionally the line ch_versions = ch_versions.mix(PYPOLCA_RUN.out.versions) in workflows/mag.nf has to be removed.
The incoming data (contigs) should be compressed as in the original code. Even if it would be necessary to allow non-compressed input (which may be ok in same cases), copying the files when already uncompressed seems like an unnecessary burden on IO and file sizes to me (assemblies in nf-core/mag can be huge!).

Generally, small changes on nf-core modules can be done and accepted by the linting via https://nf-co.re/docs/nf-core-tools/cli/modules/patch

@Harshita-sriv

Copy link
Copy Markdown
Author

@d4straub I restored the PyPOLCA module to the upstream nf-core implementation and moved the coassembly-specific logic into the workflow layer (workflows/mag.nf). The workflow now groups short reads appropriately when --coassemble_group is enabled before passing them to PyPOLCA.

The remaining CI issue appears to be the check_local_copy lint check for pypolca/run. Locally, the module matches the installed nf-core module definition (nf-core modules info pypolca/run), and the workflow compiles and reaches PYPOLCA_RUN in stub runs. All other CI checks pass.

Could you advise whether there is an additional module sync/patch step expected for this module?

@d4straub

Copy link
Copy Markdown
Collaborator

Looks good.
I think the easiest way of satisfying the linting is to re-install the module.

@dialvarezs dialvarezs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi again @Harshita-sriv! This is looking good!

A few last things:

Regarding the testing setup:
I think the best option is to use the longreadonly_alternatives but changing the samplesheet so it uses the hybrid one that has LR and SR data. Then disable SPAdes, SPAdes hybrid and MEGAHIT at least. That way we can get this tested in that profile.

Also I left a comment regarding the pypolca module requiring gzipped input (that will need updating the nf-core module).

Comment thread modules/nf-core/pypolca/run/main.nf Outdated
def read_files = reads instanceof List ? reads : [reads]
def read_file_arg = read_files.size() > 1 ? "-1 ${read_files[0]} -2 ${read_files[1]}" : "-1 ${read_files[0]}"
"""
gzip -cdf $contigs > contigs_uncompressed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you update the module so it only decompresses when the input has a .gz extension? I was testing this locally, and pypolca received the uncompressed assembly, throwing an error.
And also I think it's a better design not assuming that the input is compressed.

@Harshita-sriv Harshita-sriv Jun 15, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, I also faced this problem, but I didn't change the module because I assumed it wouldn't be accepted. However, for the testing setup, could you confirm that for test_longreadonly_alternatives, I should replace samplesheet.long_read.v4.csv with samplesheet.hybrid.v4.csv, enable run_pypolca, and keep only Flye active by disabling SPAdes, SPAdes-hybrid, MEGAHIT, and MetaMDBG?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's a totally reasonable change if you execute gzip based on the extension, so it works with compressed and uncompressed input. So don't worry about opening a PR on the module. I can review it if you ping me.

And yes, that's the setup I'm proposing. MetaMDBG is already disabled in that profile, so you'll need to disable both SPAdes and MEGAHIT.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I've implemented the requested changes:

  • Updated test_longreadonly_alternatives.config to use the hybrid samplesheet and enable run_pypolca while disabling SPAdes/SPAdesHybrid/MEGAHIT.
  • Updated the Pypolca module to support both gzipped and uncompressed assembly inputs.

The remaining issue is CI validation. The PR currently shows failures related to:

  1. The Pypolca module modification.
  2. The updated test profile / snapshot expectations.

Locally I'm unable to fully validate the nf-test because the test environment is failing before completion (missing tools such as fastp, NanoPlot, porechop), so any snapshot updates generated locally appear incomplete.

Could you please take a look and let me know how you'd like the test/snapshot updates handled, or whether the current implementation looks correct and can be reviewed from your side?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, sorry for the lack of clarification.
As Daniel said, updating the nf-core module directly in the mag repo is not the way.
To update a module you need open a PR on the modules repo: https://github.com/nf-core/modules. When your PR is accepted and merged, you can run nf-core modules update pypolca/run to update the module here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Okay, will do that. Can you also suggest about the errors due to test file modifications.

@dialvarezs dialvarezs Jun 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can ignore the errors for now. When you do the update via the official nf-core modules, the error will go away.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Okay, thank you for help!

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.

3 participants