-
Notifications
You must be signed in to change notification settings - Fork 0
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
Thermo class using primer3-py for thermodynamic calcs to replace NtThal and melting.py #110
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #110 +/- ##
==========================================
+ Coverage 97.32% 97.37% +0.04%
==========================================
Files 21 20 -1
Lines 1642 1635 -7
Branches 195 188 -7
==========================================
- Hits 1598 1592 -6
+ Misses 26 24 -2
- Partials 18 19 +1 ☔ View full report in Codecov by Sentry. |
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.
looking good to me so far
prymer/thermo.py
Outdated
annealing_temp_c: Actual annealing temperature of the PCR reaction in Celsius | ||
temp_c: Simulation temperature for structure prediction in Celsius | ||
max_nn_length: Maximum length in bases for nearest-neighbor calcs | ||
max_loop: maximum size of loops in bases when predicting structures |
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.
max_loop: maximum size of loops in bases when predicting structures | |
max_loop: Maximum size of loops in bases when predicting structures |
TM_METHOD_BRESLAUER: ClassVar[str] = "breslauer" | ||
TM_METHOD_SANTALUCIA: ClassVar[str] = "santalucia" | ||
|
||
SALT_CORRECTION_METHOD_SCHILDKRAUT: ClassVar[str] = "schildkraut" | ||
SALT_CORRECTION_METHOD_OWCZARZY: ClassVar[str] = "owczarzy" | ||
SALT_CORRECTION_METHOD_SANTALUCIA: ClassVar[str] = "santalucia" |
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.
Why not enums?
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.
I thought about it ... mostly wanted to keep the number of classes/symbols down, and felt like this was an ok compromise. That said, I'd like to expose these in PrimerAndAmpliconParameters
and ProbeParameters
and at that point enums may make better sense.
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.
I think I'd prefer enums in the Thermo
class' namespace.
prymer/api/picking.py
Outdated
@@ -119,6 +119,8 @@ def build_primer_pairs( # noqa: C901 | |||
and those exceeding the maximum Tm will be discarded | |||
weights: the set of penalty weights | |||
fasta_path: the path to the FASTA file from which the amplicon sequence will be retrieved. | |||
thermo: a Thermo instance for performing thermodynamic calculations including amplicon tm; |
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.
Does this work? It'd be nice to link to the class
thermo: a Thermo instance for performing thermodynamic calculations including amplicon tm; | |
thermo: a [`Thermo`][prymer.thermo] instance for performing thermodynamic calculations including amplicon tm; |
prymer/primer3/primer3_parameters.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.
Not sure how this happened!
result: ThermoResult = self._thermo.calc_heterodimer(bases1, bases2) | ||
return float(result.tm) | ||
|
||
def heterodimer_3p_anchored_tm(self, bases1: str, bases2: str) -> float: |
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.
why not a boolean called 3p_anchored
in heterodimer_tm
?
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.
I think it's a coin flip. You could also ask "why not a single dimer_tm()
that takes an optional second sequence. At the end of the day I think it's clearer to have separate methods with separate documentation that we can elaborate on.
TM_METHOD_BRESLAUER: ClassVar[str] = "breslauer" | ||
TM_METHOD_SANTALUCIA: ClassVar[str] = "santalucia" | ||
|
||
SALT_CORRECTION_METHOD_SCHILDKRAUT: ClassVar[str] = "schildkraut" | ||
SALT_CORRECTION_METHOD_OWCZARZY: ClassVar[str] = "owczarzy" | ||
SALT_CORRECTION_METHOD_SANTALUCIA: ClassVar[str] = "santalucia" |
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.
I think you could define the enums at the class level, to keep the namespace from being polluted.
Also consider using StrEnum
. We may want to use LowercaseStrEnum
from strenum
as StrEnum
isn't in the standard library until 3.11.
TM_METHOD_BRESLAUER: ClassVar[str] = "breslauer" | |
TM_METHOD_SANTALUCIA: ClassVar[str] = "santalucia" | |
SALT_CORRECTION_METHOD_SCHILDKRAUT: ClassVar[str] = "schildkraut" | |
SALT_CORRECTION_METHOD_OWCZARZY: ClassVar[str] = "owczarzy" | |
SALT_CORRECTION_METHOD_SANTALUCIA: ClassVar[str] = "santalucia" | |
@enum.unique | |
class TmMethod(Enum): | |
BRESLAUER = "breslauer" | |
SANTALUCIA = "santalucia" | |
@enum.unique | |
class SaltCorrectionMethod(Enum): | |
SCHILDKRAUT = "schildkraut" | |
OWCZARZY = "owczarzy" | |
SANTALUCIA = "santalucia" |
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.
I'm going to defer this for now. I hear you .. but actually the way these same things get provided to primer3 during design is as integers (i.e. 0, 1, or 2). So I want to think about where these belong and how best to manage them when I circle back and add them there.
the melting temperature of the most likely (lowest energy) 3' anchored heterodimer | ||
""" | ||
result: ThermoResult = self._thermo.calc_end_stability(bases1, bases2) | ||
return float(result.tm) |
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.
Not covered due to copy-paste typo below
tests/test_thermo.py
Outdated
salt_methods = [ | ||
Thermo.SALT_CORRECTION_METHOD_SANTALUCIA, | ||
Thermo.SALT_CORRECTION_METHOD_SCHILDKRAUT, | ||
Thermo.SALT_CORRECTION_METHOD_OWCZARZY | ||
] | ||
|
||
tm_methods = [ | ||
Thermo.TM_METHOD_SANTALUCIA, | ||
Thermo.TM_METHOD_BRESLAUER | ||
] |
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.
enums would avoid this, just saying.
Co-authored-by: Nils Homer <[email protected]>
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.
Added the enum work into #108, as I moderately want it
Creates a this wrapper around primer3-py's
_ThermoAnalysis
allowing us to add typing, docs, etc. Then replaces NtThal and melting.py with the new implementation.