Dsl2 add sharding of fastqs before alignment #1023
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I'm looking into the bug here. Seems to be when --skip_preprocessing is used. |
TCLamnidis
reviewed
Aug 25, 2023
There was a problem hiding this comment.
Good progress! 🚀 Just a few small things:
- Check sharded BAM headers. If they are full of RGs for each shard, we might want to tweak that so the RGs are still merged into one per lane as if sharding did not happen. My reasoning is that sharding is a computational optimisation, but does not alter the result itself. @jfy133 @shyama-mama your thoughts?
- Rename sharding parameters (can be done once code is mostly done).
- Potentially superfluous module include for
SAMTOOLS_MERGE_SHARDS
…//github.com/nf-core/eager into dsl2-add-sharding-of-fastqs-before-alignment
|
TCLamnidis
requested changes
Sep 8, 2023
…mapping without overwriting reads channel.
|
Not sure what the issue here is with linting. Unable to fix it automatically. |
|
@shyama-mama I think the issue with the linting had to do with the new template that has been merged now. Could you resolve the conflicts? Then I can review again and we merge this, hopefully 🦾 😄 |
TCLamnidis
requested changes
Nov 3, 2023
There was a problem hiding this comment.
LGTM.
One minor comment about the groupTuple call: I'm not sure if it is needed or not. But if it works it works.
Only outstanding thing is to add tests in the CI for sharding. Could you please activate sharding for one of the CI jobs with a small number of reads per shard (1000)? File is at .github/workflows/ci.yml
|
Would I have to add the test in this branch and push changes for the test to run here? Should I also add tests for sharding with bowtie2 and bwamem options for mapping? |
…//github.com/nf-core/eager into dsl2-add-sharding-of-fastqs-before-alignment
|
Add the test here, and push and it will run on this branch, yes. |
|
@TCLamnidis Actually I realised I've got sharding enabled with 5000 reads as part of the 'test' profile in the config in this branch. So it has been running for all these tests. Would you rather have it added explicitly in the ci.yml with one of the tests? |
|
oh right! Skimmed over that >.< nevermind me then. undo the ci.yml changes! all good |
TCLamnidis
approved these changes
Nov 3, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR checklist
Added functionality to shard fastqs before aligning. This uses SeqKit. The sharded fastqs are merged together during the lane merge step.
scrape_software_versions.pynf-core lint .).nextflow run . -profile test,docker).docs/usage.mdis updated.docs/output.mdis updated.CHANGELOG.mdis updated.README.mdis updated (including new tool citations and authors/contributors).