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

[feature] TrimPrimers can trim only R1s #681

Merged
merged 2 commits into from
Aug 10, 2021

Conversation

nh13
Copy link
Member

@nh13 nh13 commented Jun 18, 2021

Alternative to #679. The advantage of this PR is it's a bit smaller of a modification. The negative is using -1 as a proxy for None. In >90% of cases I'd prefer to use Some/None, but I think this is simple enough of a change to make it work with TrimPrimers

@FelixMoelder
Copy link

FelixMoelder commented Jul 5, 2021

Thanks for offering an alternative variant of single primer trimming.
@dlaehnemann has created a small set of test data which we applied on your and our implementation (see #679).
Performing a diff on both results showed that reads where an amplicon was found are trimmed identical by both implementations.
In case no amplicon can be found our implementation does some weired stuff trimming both ends of a read by the maximum primer length.
We did not investigate this further as it is obviously a bug which does not occur in your implementation.

Regardless on comparing both results we also checked if the reads are trimmed as we would except (and double checked the more complex cases).
It looks like everything is fine so far and the more complex cases (leading softclips, extremely short reads, reads w/o amplicon) are also handled correctly.

Therefore, we suggest merging your implementation as it is also much cleaner and we don't see a reason trying to fix ours.

Test_primers.zip

@nh13
Copy link
Member Author

nh13 commented Jul 9, 2021

@FelixMoelder thank-you for testing this implementation, we'll get it code reviewed and merged!

@FelixMoelder
Copy link

@nh13 Can you estimate when this will be merged?

@nh13 nh13 requested a review from tfenne August 9, 2021 22:09
@nh13 nh13 force-pushed the feature/trim-primers-first-of-pair branch from 728dd75 to 2b2a3f2 Compare August 9, 2021 22:10
@nh13 nh13 merged commit e91698c into master Aug 10, 2021
@nh13 nh13 deleted the feature/trim-primers-first-of-pair branch August 10, 2021 16:36
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