-
Notifications
You must be signed in to change notification settings - Fork 13
added fasta parsing for compatible primers and revisited primer dimer evaluation #57
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
Conversation
varvamp/scripts/qpcr.py
Outdated
|
|
||
| def forms_dimer_or_overhangs(right_primer, left_primer, probe, ambiguous_consensus): | ||
| """ | ||
| checks if combinations of primers/probe form dimers or overhangs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs better explanation that now its a check for all permutations
|
This now implements more stringent primer dimer considerations. Also the primer dimer solve at the end of the scheme is now more stringent. For each potential pool of alternate primers the primer cloud is first checked against all primers in the scheme that do not lead to primer dimers to inhibit that primer switching produces new primer dimers with existing primers. By that we only switch primers when it we are sure that new primer dimers can only occur from primer dimers in the primers we want to switch. As this is computationally more extensive for larger schemes, multiprocessing was implemented. @wm75 If you have time to check this also, I would be super happy. In any case, because I changed some settings in the config, the Galaxy tool likely has to be slightly modified. |
|
and moreover added multi-processing at different steps of the respective workflows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds support for filtering newly designed primers against a user-provided FASTA of “compatible primers”, tightens primer-dimer evaluation (Tm, ΔG, and end-overlap checks), and introduces multiprocessing to reduce runtime impact.
Changes:
- Add
--compatible-primersFASTA input and filter primer/probe candidates against it. - Replace direct
calc_dimer().tmchecks with a unifiedprimers.is_dimer()(Tm/ΔG/end-overlap). - Parallelize several expensive steps (primer/probe search, dimer resolution).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
varvamp/scripts/scheme.py |
Uses primers.is_dimer() and parallelizes heterodimer resolution; returns richer dimer info. |
varvamp/scripts/reporting.py |
Changes dimer report output format and includes structure/ΔG. |
varvamp/scripts/qpcr.py |
Parallelizes probe discovery and probe→amplicon assessment; switches to primers.is_dimer(). |
varvamp/scripts/primers.py |
Adds FASTA parsing for external primers, new dimer logic (is_dimer), and multiprocessing primer filtering. |
varvamp/scripts/logging.py |
Updates config validation for new params and extends goodbye messages. |
varvamp/scripts/default_config.py |
Introduces PRIMER_MAX_DIMER_DELTAG and moves END_OVERLAP into primer dimer constraints; lowers default max dimer Tm. |
varvamp/command.py |
Adds CLI arg, wires new multiprocessing signatures, integrates compatible-primer filtering and updated dimer solving output. |
docs/how_varvamp_works.md |
Documents new multiprocessing behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
varvamp/command.py:152
- In
qpcrmode,--thresholdis no longer required and now silently defaults to 0.9. This is a CLI behavior change that isn’t mentioned in the PR description and may surprise users/scripts that relied on the previous requirement. If intentional, please call it out in release notes/docs; otherwise consider restoringrequired=True.
QPCR_parser.add_argument(
"-t",
"--threshold",
metavar="0.9",
type=float,
default=0.9,
help="consensus threshold (0-1) - higher values result in higher specificity at the expense of found primers"
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Finally, regarding https://github.com/jonas-fuchs/varVAMP/pull/57/changes#r2732047859: I guess you can write the primer dimer report as part of varVAMP/varvamp/scripts/reporting.py Lines 300 to 305 in 9c9f99f
you have both the final ( primer_name) and the original (primer[-1]) primer name available.So, for each primer you're writing to that tsv, you could check based on its original name whether it's among the dimers, then write it to the dimer file under it's new name. |
Co-authored-by: Wolfgang Maier <[email protected]>
Co-authored-by: Wolfgang Maier <[email protected]>
Co-authored-by: Wolfgang Maier <[email protected]>
Co-authored-by: Wolfgang Maier <[email protected]>
Co-authored-by: Wolfgang Maier <[email protected]>
Co-authored-by: Wolfgang Maier <[email protected]>
…e_compatibility # Conflicts: # varvamp/scripts/qpcr.py
|
@wm75 Thanks a lot for your review. If you find the time to double check after the implementations, it would be great! |
on it. |
varvamp/scripts/qpcr.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, I'd change this to:
# Create an iterator over the first n amplicons for processing
# Amplicons are sorted first on whether offset targets were predicted for them,
# then by penalty. This ensures that amplicons with offset targets are always considered last.
amplicons = itertools.islice(
sorted(qpcr_schemes_candidates, key=lambda x: (x.get("offset_targets", False), x["penalty"])),
n_to_test
)
batch_size = int(n_to_test/n_processes)
callable_f = functools.partial(
process_single_amplicon_deltaG,
majority_consensus=majority_consensus
)
# Create a pool of processes to handle the concurrent processing
with multiprocessing.Pool(processes=n_processes) as pool:
# process amplicons concurrently
results = pool.map(callable_f, amplicons, batch_size)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All code changes look good and consistent to me.
I've tested various command line runs and all seems good.
What I didn't check is whether all changes are properly documented.
varvamp/scripts/primers.py
Outdated
| import functools | ||
|
|
||
| # LIBS | ||
| from Bio.Seq import Seq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| from Bio.Seq import Seq | |
| from Bio.Seq import MutableSeq |
varvamp/scripts/primers.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return str(MutableSeq(seq).reverse_complement(inplace=True)) |
just a simple micro-optimization that occurred to me while going through the code. Makes only a few seconds runtime difference overall.
|
I'll carefully recheck documentation. Thanks alot for the review :)
|
This PR allows to parse a fasta file with primers that should not form dimers with the new scheme and will hopefully allow the design of multi-schemes. Moreover it applies more stringent primer dimer filtering. First, primer dimer standrad temp was set to a lower temp and then primer end overlaps are always checked for all primers not only qpcr. as the parsed fasta can increase the computation time dramatically for schemes with a lot of primers, multiprocessing was implemented.