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.

Sign up for GitHub

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

refactor: Make PrimerPair.amplicon a cached property #93

Merged
merged 2 commits into from
Nov 20, 2024
Merged

refactor: Make PrimerPair.amplicon a cached property #93

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d6e40fd
chore: exclude site/ and docs/ from mypy
msto Nov 18, 2024
6fcd304
refactor: make amplicon a cached property
msto Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 4 additions & 15 deletions prymer/api/primer_pair.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ class methods to represent a primer pair. The primer pair is comprised of a lef
""" # noqa: E501

from dataclasses import dataclass
from dataclasses import field
from dataclasses import replace
from functools import cached_property
from typing import Iterator
from typing import Optional

Expand All @@ -55,7 +55,7 @@ class methods to represent a primer pair. The primer pair is comprised of a lef
from prymer.api.span import Span


@dataclass(frozen=True, init=True, kw_only=True, slots=True)
@dataclass(frozen=True, init=True, kw_only=True)
class PrimerPair(OligoLike):
"""
Represents a pair of primers that work together to amplify an amplicon. The
Expand All @@ -79,22 +79,11 @@ class PrimerPair(OligoLike):
amplicon_tm: float
penalty: float
amplicon_sequence: Optional[str] = None
_amplicon: Span = field(init=False)

def __post_init__(self) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here is that the validation that the LP/RP are oriented sensibly with respect to each other now won't run until/unless a user accesses the amplicon span.

Copy link
Collaborator Author

@msto msto Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think that’s true, as evidenced by these tests continuing to pass.

prymer/tests/api/test_primer_pair.py

Lines 422 to 457 in ebc72f7

def test_reference_mismatch() -> None:
"""
Test that a PrimerPair and both Primers all have a Span with
the same reference sequence
"""
pp = PRIMER_PAIR_TEST_CASES[0].primer_pair
with pytest.raises(ValueError, match="different references"):
replace(
pp,
left_primer=replace(
pp.left_primer,
span=replace(pp.left_primer.span, refname="no-name"),
),
)
with pytest.raises(ValueError, match="different references"):
replace(
pp,
right_primer=replace(
pp.right_primer,
span=replace(pp.right_primer.span, refname="no-name"),
),
)
def test_right_primer_before_left_primer() -> None:
"""Test that an exception is raised if the left primer starts after the right primer ends"""
pp = PRIMER_PAIR_TEST_CASES[0].primer_pair
with pytest.raises(ValueError, match="Left primer does not start before the right primer"):
replace(
pp,
left_primer=pp.right_primer,
right_primer=pp.left_primer,
)

Post-init checks perform the validation at the time of instantation. In this case, the validation is in the parent class’s post-init. The post-init for OligoLike references the instance’s span, which just returns the value of amplicon (I’ve previously suggested removing such synonymous properties so this kind of logic is less obfuscated) and therefore calls the validation code in question.

prymer/prymer/api/oligo_like.py

Line 59 in ebc72f7

elif self.span.length != len(self.bases):

I think this is bad design for a number of reasons. One, abstract base classes should be abstract. They should declare an interface but not provide implementation. Two, this is an example of DRY going bad in practice - the logic for validating a primer pair is different from validating a single oligo, but we’ve forced both to use the same code. Three, hiding the logic away from the dataclass where it’s used will naturally lead to the kind of confusion you just ran into.

I would also like to refactor to fix that, but in the interest of not pulling threads upon threads, I kept this PR more tightly scoped.

Copy link
Collaborator Author

@msto msto Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tl;dr The validation logic is still called, and further refactoring would be related to #57 and #77

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, ok. So a potential foot-gun to be dealt with as soon as we deal with PrimerLike.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

# Derive the amplicon from the left and right primers. This must be done before
# calling super() as `PrimerLike.id` depends on the amplicon being set
object.__setattr__(
self,
"_amplicon",
PrimerPair.calculate_amplicon_span(self.left_primer, self.right_primer),
)
super(PrimerPair, self).__post_init__()

@property
@cached_property
def amplicon(self) -> Span:
"""Returns the mapping for the amplicon"""
return self._amplicon
return self.calculate_amplicon_span(self.left_primer, self.right_primer)

@property
def span(self) -> Span:
Expand Down
1 change: 1 addition & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ warn_return_any = true
warn_unreachable = true
warn_unused_configs = true
warn_unused_ignores = true
exclude = ["site/", "docs/"]

[[tool.mypy.overrides]]
module = "defopt"
Expand Down
Loading