Merge Primer*Weights into Primer3Parameters, remove Primer3Input #112
Conversation
prymer/primer3/primer3_parameters.py
Outdated
| product_size_lt: weight for products shorter than | ||
| `PrimerAndAmpliconParameters.amplicon_sizes.opt` | ||
| product_size_gt: weight for products longer than | ||
| `PrimerAndAmpliconParameters.amplicon_sizes.opt` |
There was a problem hiding this comment.
The frequency with which these come in pairs makes me think maybe we'd want to have a WeightPair (better name needed) class like MinOptMax so that you could do:
amplicon_size_wts = Weights(0.5, 1.5)There was a problem hiding this comment.
Also two other thoughts:
- I think the weights should match naming with above, so e.g. where we've said "amplicon_tms" we should use "amplicon" instead of "product" in the weights (or vice versa).
- I think for clarity all weights should either have a
wt_prefix or_wtsuffix; some do and some don't here.
prymer/primer3/primer3_parameters.py
Outdated
| target: Span | ||
| task: Primer3TaskType |
There was a problem hiding this comment.
Since these are in the super class, do you need to repeat them here?
prymer/primer3/primer3_parameters.py
Outdated
| def as_amplicon_params(self) -> "AmpliconParameters": | ||
| """Use this method when you want to treat these parameters as amplicon parameters.""" | ||
| if isinstance(self, AmpliconParameters): | ||
| return typing.cast(AmpliconParameters, self) | ||
| raise Exception("The parameters are not amplicon parameters") | ||
|
|
||
| def as_probe_params(self) -> "ProbeParameters": | ||
| """Use this method when you want to treat these parameters as probe parameters.""" | ||
| if isinstance(self, ProbeParameters): | ||
| return typing.cast(ProbeParameters, self) | ||
| raise Exception("The parameters are not amplicon parameters") |
There was a problem hiding this comment.
This feels out of place to me - why not just check and cast where that's needed rather than having it in the superclass?
prymer/primer3/primer3.py
Outdated
| ): # if the right primer has too many dinucleotide bases, fail it | ||
| dinuc_pair_failures.append(primer_pair.right_primer) | ||
| valid = False | ||
| if valid: # if neither failed, append the pair to a list of valid designs | ||
| valid_primer_pair_designs.append(primer_pair) | ||
| return valid_primer_pair_designs, dinuc_pair_failures | ||
|
|
||
| def design(self, design_input: Primer3Input) -> Primer3Result: # noqa: C901 | ||
| def design(self, design_input: Primer3Parameters) -> Primer3Result: # noqa: C901 |
There was a problem hiding this comment.
I think this would need to take:
amplicon_parameters: AmpliconParameters | None, probe_parameter: ProbeParameters | NoneWhat you have works for the current set of use cases, but one primer3 task we haven't implemented yet supports designing primer pairs and probes concurrently, and then you need both sets of parameters.
There was a problem hiding this comment.
I leave it for now, and we can decide later if we want to have two parameters to design, or to create a third parameters class:
class AmpliconAndProbeParameters(Primer3Parameters):
amplicon_params: AmpliconParams
probe_params: ProbeParams
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #112 +/- ##
==========================================
- Coverage 97.37% 96.83% -0.54%
==========================================
Files 20 18 -2
Lines 1635 1579 -56
Branches 188 185 -3
==========================================
- Hits 1592 1529 -63
- Misses 24 28 +4
- Partials 19 22 +3 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
nh13
changed the title
| @@ -731,29 +754,3 @@ def _check_design_results(design_input: Primer3Input, design_results: dict[str, | |||
| count: int = int(maybe_count) | |||
|
|
|||
| return count | |||
|
|
|||
|
|
|||
| def _has_acceptable_dinuc_run(design_input: Primer3Input, oligo_design: Oligo) -> bool: | |||
| @@ -30,7 +30,7 @@ | |||
| from prymer.model import Oligo | |||
| from prymer.model import PrimerPair | |||
| from prymer.model import Span | |||
| from prymer.primer3 import PrimerAndAmpliconWeights | |||
| from prymer.primer3 import AmpliconParameters | |||
There was a problem hiding this comment.
If we're going to rename it from PrimerAndAmplicon I'd vote for PrimerParameters as it feels like the majority of the parameters are around primers. Also, can be used for designing just left primers (no amplicon), just right primers (no amplicon) or both (and therefore an amplicon).
| @@ -83,6 +83,55 @@ def __str__(self) -> str: | |||
| return f"(min:{self.min}, opt:{self.opt}, max:{self.max})" | |||
|
|
|||
|
|
|||
| @dataclass(slots=True, frozen=True, init=True) | |||
| class WeightRange(Generic[Numeric]): | |||
There was a problem hiding this comment.
I like this, but a few thoughts:
- It's not really a range, more of a pair. I.e. I can have a higher penalty weight for "primer tm too low" than "primer tm too high" - they're independent values. Perhaps it's just
Weights? - I think in p3 weights are always floats because they are scaling factors, so i think we could un-generify it and juse always have the values be floats
Primer*Weightsclasses into their corresponding amplicon and probe parameter classes (reduces 4 classes to two). The attributes have the_wtsuffix in their name. Theprimer3.primer3.primer3_weightsmodule is gone.Primer3Inputfunctionality intoPrimer3directly.primer3.primer3.primer3_inputis now gone.WeightRangefor the weights when a value is out of range (either less than, or greater than). Could use a better name?Primer3.designnow explicitly takes in the design task and target, in addition to the params. It checks to make sure that the task (e.g. design left primers) matches the params (e.g. amplicons).Primer3Task(remove methods no longer used)