Make sure the number_probes_returned is respected. #79
Conversation
prymer/primer3/primer3_parameters.py
Outdated
| @@ -219,6 +219,7 @@ def to_input_tags(self) -> dict[Primer3InputTag, Any]: | |||
| Primer3InputTag.PRIMER_INTERNAL_MAX_GC: self.probe_gcs.max, | |||
| Primer3InputTag.PRIMER_INTERNAL_MAX_POLY_X: self.probe_max_polyX, | |||
| Primer3InputTag.PRIMER_INTERNAL_MAX_NS_ACCEPTED: self.probe_max_Ns, | |||
| Primer3InputTag.PRIMER_NUM_RETURN: self.number_probes_return | |||
There was a problem hiding this comment.
How hard would it be to implement a unit test to catch a potential regression in the future?
There was a problem hiding this comment.
I agree this would be nice, but there is not an existing test to amend, and I don't have the time to also fix the test shortcomings at this moment.
WalkthroughThe pull request introduces several changes to the In the The 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)tests/primer3/test_primer3_input.py (1)
The new assertions properly verify that:
This aligns well with the PR objective of ensuring Let's verify the relationship between this test and the implementation: ✅ Verification successfulTest coverage correctly validates probe number parameter implementation The verification confirms that:
The test changes align perfectly with the implementation, providing appropriate coverage for the probe number validation. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify the ProbeParameters class includes number_probes_returned
# and that it's properly mapped to PRIMER_NUM_RETURN
# Test 1: Check ProbeParameters class definition
rg -A 10 "class ProbeParameters"
# Test 2: Check the mapping implementation
rg "PRIMER_NUM_RETURN" --type py
Length of output: 1718 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
prymer/primer3/primer3_parameters.py (1)
222-222: Add validation for number_probes_return.While the implementation aligns well with PrimerAndAmpliconParameters, consider adding validation in post_init to ensure number_probes_return is positive.
Add this check to post_init:
def __post_init__(self) -> None: if not isinstance(self.probe_sizes.min, int): raise TypeError("Probe sizes must be integers") if not isinstance(self.probe_tms.min, float) or not isinstance(self.probe_gcs.min, float): raise TypeError("Probe melting temperatures and GC content must be floats") if self.probe_max_dinuc_bases % 2 == 1: raise ValueError("Max threshold for dinucleotide bases must be an even number of bases") + if self.number_probes_return <= 0: + raise ValueError("number_probes_return must be positive")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
prymer/primer3/primer3_parameters.py(1 hunks)
🔇 Additional comments (2)
prymer/primer3/primer3_parameters.py (2)
222-222: LGTM! This change ensures number_probes_return is respected.
The addition of PRIMER_NUM_RETURN to the mapped dictionary correctly propagates the number_probes_return parameter to Primer3, matching the behavior in PrimerAndAmpliconParameters.
222-222: Verify test coverage for number_probes_return.
Let's ensure there's adequate test coverage for this change.
✅ Verification successful
Test coverage exists and implementation is consistent
The verification shows:
- Test coverage exists in
tests/primer3/test_primer3_parameters.pywhich verifies thenumber_probes_returnvalue - The
PRIMER_NUM_RETURNtag is used consistently across the codebase:- As an enum in
primer3_input_tag.py - In parameter mapping in
primer3_parameters.py - With consistent default value of 5 in both implementation and tests
- As an enum in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test coverage and potential conflicts
# Test 1: Look for existing tests of number_probes_return
echo "Checking for existing tests..."
rg -l "number_probes_return" --type py --glob "test_*.py"
# Test 2: Check for any direct usage of PRIMER_NUM_RETURN that might conflict
echo "Checking for direct PRIMER_NUM_RETURN usage..."
rg "PRIMER_NUM_RETURN" --type py --exclude "primer3_parameters.py"
Length of output: 708
Script:
#!/bin/bash
# Let's try a different approach to search for tests and usages
# Search for test files containing number_probes_return
echo "=== Searching for test coverage ==="
rg "number_probes_return" -C 3 --type py
# Search for PRIMER_NUM_RETURN usages
echo -e "\n=== Searching for PRIMER_NUM_RETURN usages ==="
rg "PRIMER_NUM_RETURN" -C 3 --type py
Length of output: 5364
There was a problem hiding this comment.
Hey @tfenne
Thanks for catching this and pushing a fix. I noticed there's another attribute on the parameters dataclass (probe_max_dinuc_bases) that isn't propagated to the input tag dict.
Should we review all the parameter dataclasses to ensure that all included parameters are represented in the corresponding input tag dicts?
Also, how did you and Nils determine which subset of primer3 input tags to expose in fgprimer and prymer? I noticed there are several tags relevant to probe design which we don't include. e.g. related to your comments yesterday about ensuring the melting temperature calculation receives the same parameters as IDT uses, prymer does not currently permit the user to specify PRIMER_INTERNAL_SALT_MONOVALENT or PRIMER_INTERNAL_SALT_DIVALENT, or DMSO/DNA/formamide concentration
prymer/primer3/primer3_parameters.py
Outdated
| @@ -219,6 +219,7 @@ def to_input_tags(self) -> dict[Primer3InputTag, Any]: | |||
| Primer3InputTag.PRIMER_INTERNAL_MAX_GC: self.probe_gcs.max, | |||
| Primer3InputTag.PRIMER_INTERNAL_MAX_POLY_X: self.probe_max_polyX, | |||
| Primer3InputTag.PRIMER_INTERNAL_MAX_NS_ACCEPTED: self.probe_max_Ns, | |||
| Primer3InputTag.PRIMER_NUM_RETURN: self.number_probes_return | |||
There was a problem hiding this comment.
note probe_max_dinuc_bases is also not represented in the input tag mapping
There was a problem hiding this comment.
This one is actually on purpose - primer3 doesn't do dinuc checking. It's definitely slightly confusing that we then supply the parameter is something that makes it look like it's going to primer3, but the check is done separately after primer3.
|
FYI I'd like to better understand the motivation for limiting these dataclasses to a subset of the available tags/params. It looks like |
tfenne
force-pushed
the
tf_probes_returned
branch
from
9edd3be to
dcaf38a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #79 +/- ##
=======================================
Coverage 96.74% 96.74%
=======================================
Files 26 26
Lines 1751 1751
Branches 333 333
=======================================
Hits 1694 1694
Misses 31 31
Partials 26 26 ☔ View full report in Codecov by Sentry. |
No description provided.