Skip to content
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

Add chopper and nanoq options for longread preprocessing #692

Merged
merged 18 commits into from
Nov 22, 2024

Conversation

muabnezor
Copy link
Contributor

@muabnezor muabnezor commented Oct 14, 2024

closes #661

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

…subworkflow for alternative use of chopper for lambda-removal instead of nanolyse, and nanoq for longread filtering instead of filtlong
@muabnezor muabnezor added the WIP Work in progress label Oct 14, 2024
@muabnezor muabnezor requested a review from jfy133 October 14, 2024 13:22
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.

Code looking good so far!

Missing updates to citations.md and output.md though!

nextflow_schema.json Show resolved Hide resolved
subworkflows/local/longread_preprocessing.nf Outdated Show resolved Hide resolved
@jfy133
Copy link
Member

jfy133 commented Oct 14, 2024

Also don't worry about linting and checkm tests, the format I'm fixing now in the template merge in #689 , and for checkM the database server is done

@jfy133 jfy133 marked this pull request as draft October 14, 2024 16:01
conf/modules.config Outdated Show resolved Hide resolved
@muabnezor
Copy link
Contributor Author

I realized that allowing the user to choose filtering tools other than filtlong for long reads might introduce an issue. Since filtlong now uses preprocessed short reads to filter the long reads, this means that long reads will piggyback on the short reads and also undergo host removal, assuming the host is removed from the short reads.

If the user opts to use chopper for filtering long reads, we might need to implement a separate process for host removal from the long reads. However, it may be better to address this in a future PR. What do you think, @jfy133?

@muabnezor muabnezor self-assigned this Oct 15, 2024
@jfy133
Copy link
Member

jfy133 commented Oct 15, 2024

Hmm, might need some discussion when I'm not so tired, that said the issue might be related to #691, is that correct? Maybe if we update the filtlong module this is less of a problem? What do you think?

@muabnezor
Copy link
Contributor Author

Hmm, might need some discussion when I'm not so tired, that said the issue might be related to #691, is that correct? Maybe if we update the filtlong module this is less of a problem? What do you think?

#691 is kind of related to this, but I think it is reasonable to use when running in hybrid mode. Maybe we can talk more during dev-hours?

@jfy133
Copy link
Member

jfy133 commented Oct 17, 2024

Yes let's talk then!

I've almost finished the template merge too so can be more involved after that too 👍

@muabnezor muabnezor added ready to review and removed WIP Work in progress labels Nov 1, 2024
@muabnezor muabnezor marked this pull request as ready for review November 1, 2024 10:56
@muabnezor
Copy link
Contributor Author

This should be ready for review @jfy133

@jfy133
Copy link
Member

jfy133 commented Nov 1, 2024

Woohoo! I'm travelling rest of today and travelling again second half next week, I'll try and slip it in Mon/Tuesday if I can

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 all looks fine and functional - any remaining structural questions as up above can be addressed in a follow up PR if it becomes an issue :)

Thanks again for a very clean PR @muabnezor !

Minor correcitons and good to merge :)

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
conf/modules.config Outdated Show resolved Hide resolved
docs/output.md Outdated Show resolved Hide resolved
nextflow.config Outdated Show resolved Hide resolved
Co-authored-by: James A. Fellows Yates <[email protected]>
muabnezor and others added 5 commits November 22, 2024 08:05
Co-authored-by: James A. Fellows Yates <[email protected]>
Co-authored-by: James A. Fellows Yates <[email protected]>
Co-authored-by: James A. Fellows Yates <[email protected]>
Co-authored-by: James A. Fellows Yates <[email protected]>
@muabnezor muabnezor merged commit ebb4283 into nf-core:dev Nov 22, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants