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

feat: allow for left/right primer hits to overlap when building primer pair hits with OverlapDetector #102

Open
wants to merge 6 commits into
base: am_overlap_detector_post_init_validation
Choose a base branch
from

Conversation

ameynert
Copy link
Contributor

@ameynert ameynert commented Jan 1, 2025

See discussion.

The code before this PR does not return primer pair hits where the hits to the left and right primers overlap, and does not specify a minimum acceptable amplicon size.

This change adds two new parameters to the OffTargetDetector to address this:

  • allow_overlapping_hits - defaults to False to maintain current behaviour. If this is set to True, then primer pair hits where the left and right primer hits overlap are allowed, as long as they meet the acceptable amplicon size criteria.
  • min_amplicon_size - defaults to 1 to maintain current behaviour, and is overridden by allow_overlapping_hits=False.

Copy link

codecov bot commented Jan 1, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.81%. Comparing base (c9075ac) to head (22fc66f).

Additional details and impacted files
@@                            Coverage Diff                            @@
##           am_overlap_detector_post_init_validation     #102   +/-   ##
=========================================================================
  Coverage                                     96.81%   96.81%           
=========================================================================
  Files                                            26       26           
  Lines                                          1787     1792    +5     
  Branches                                        215      216    +1     
=========================================================================
+ Hits                                           1730     1735    +5     
  Misses                                           31       31           
  Partials                                         26       26           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ameynert ameynert force-pushed the am_allow_overlapping_hits branch from dce889a to 434218d Compare January 7, 2025 17:57
@ameynert ameynert changed the base branch from am_primer_pair_hit_building to am_overlap_detector_post_init_validation January 7, 2025 17:58
@ameynert ameynert force-pushed the am_allow_overlapping_hits branch from 434218d to e39670e Compare January 7, 2025 18:02
@ameynert ameynert marked this pull request as ready for review January 7, 2025 18:30
@ameynert ameynert requested review from nh13 and tfenne as code owners January 7, 2025 18:30
@ameynert ameynert self-assigned this Jan 7, 2025
@tfenne
Copy link
Member

tfenne commented Jan 10, 2025

I took a quick look at this ... and I'm leaning towards thinking that we should:
a) not even parameterize, but always allow overlapping hits
b) default the "min amplicon size" to something slightly higher, e.g. 20bp

@nh13 and @ameynert what do you think of that?

@ameynert ameynert force-pushed the am_overlap_detector_post_init_validation branch from 1a61cb0 to c9075ac Compare January 10, 2025 23:06
@ameynert ameynert force-pushed the am_allow_overlapping_hits branch from cfaca8f to 22fc66f Compare January 10, 2025 23:14
@ameynert
Copy link
Contributor Author

I took a quick look at this ... and I'm leaning towards thinking that we should: a) not even parameterize, but always allow overlapping hits b) default the "min amplicon size" to something slightly higher, e.g. 20bp

@nh13 and @ameynert what do you think of that?

I agree - I implemented it this way with the intention of not making a breaking change, but I think a user is more likely to not care too much if the primers overlap a bit so long as they get a long enough amplicon.

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.

2 participants