Skip to content

Replace pool read includes with cat fastq modules for improved handling of single and paired reads #819

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

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

Brijeshthummar02
Copy link
Member

@Brijeshthummar02 Brijeshthummar02 commented May 29, 2025

Closes #810

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).

Copy link

This PR is against the main branch ❌

  • Do not close this PR
  • Click Edit and change the base to dev
  • This CI test will remain failed until you push a new commit

Hi @Brijeshthummar02,

It looks like this pull-request is has been made against the Brijeshthummar02/mag main branch.
The main branch on nf-core repositories should always contain code from the latest release.
Because of this, PRs to main are only allowed if they come from the Brijeshthummar02/mag dev branch.

You do not need to close this PR, you can change the target branch to dev by clicking the "Edit" button at the top of this page.
Note that even after this, the test will continue to show as failing until you push a new commit.

Thanks again for your contribution!

@Brijeshthummar02 Brijeshthummar02 changed the base branch from main to dev May 29, 2025 03:18
…ng of single and paired reads

Signed-off-by: Brijeshthummar02 <[email protected]>
Copy link
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

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

I think this case I would just like to replace the module name where it's invoked (rather than using an alias)

Also you can remove the old local modules too if we are no longer using them :)

@nf-core nf-core deleted a comment from jfy133 Jun 1, 2025
@Brijeshthummar02
Copy link
Member Author

I think this case I would just like to replace the module name where it's invoked (rather than using an alias)

Also you can remove the old local modules too if we are no longer using them :)

Fixed

@jfy133
Copy link
Member

jfy133 commented Jun 1, 2025

Sorry @Brijeshthummar02 , by invoked I mean where the module is also used. I realise my suggestions weren't clear

So you need to do the following:

  1. Have two include lines, with aliases of CAT_FASTQ_SHORT and CAT_FASTQ_LONG (before I meant don't retain the old name) around line 51 of mag.nf
  2. Then replace the contents of the else between 356-363, and 364 with simply CAT_FASTQ_SHORT (don't need to use the old filters nor the mix), and possibly update the branch name used for assigning ch_short_reads_spades cahnnel
  3. Replace lines 372-373 with CAT_FASTQ_LONG` accordingly
  4. Delete the two modules/local/pool_* module filess

@Brijeshthummar02
Copy link
Member Author

Brijeshthummar02 commented Jun 12, 2025

@jfy133 all code is missing after cfc76dc
PR is empty now.

@jfy133
Copy link
Member

jfy133 commented Jun 12, 2025

@jfy133 all code is missing after cfc76dc
PR is empty now.

Oops sorry @Brijeshthummar02 I forgot to say that the pool reads modules have moved from the main workflow to the short read preprocessing subworkflow!

Really sorry about that, you started just before a big bunch of refactoring - that's my fault for not warning you (I'm spinning too many plates at the moment)

@Brijeshthummar02
Copy link
Member Author

Should i reopen the PR.?

@dialvarezs
Copy link
Contributor

Hi @Brijeshthummar02. The code has been reorganized, but the issue is still not resolved. You can continue working on this PR using the current dev branch. Look for POOL_* module usages in the subworkflows, not in mag.nf.

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