prymer/api/__init__.py (2)

13-15: Remove commented imports.

Delete commented code instead of keeping it as comments.

-# from prymer.api.variant_lookup import _DiskBasedLookup
-# from prymer.api.variant_lookup import _InMemoryLookup
-# from prymer.api.variant_lookup import _VariantLookup

---

API changes need attention - internal classes still in use

The commented-out classes (_DiskBasedLookup, _InMemoryLookup, _VariantLookup) are actively used in:

• prymer/primer3/primer3.py
• tests/primer3/test_primer3.py
• tests/api/test_variant_lookup.py
• prymer/api/variant_lookup.py

These classes are implementation details still required by the codebase.

🔗 Analysis chain

Line range hint 1-35: Verify API changes don't break existing code.

API changes align with PR objectives but need verification.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Find usage of removed exports
rg "FileBasedVariantLookup|VariantOverlapDetector|cached|disk_based" --type python

Length of output: 115

---

Script:

#!/bin/bash
# Search for usage of removed exports in Python files
rg -t py "FileBasedVariantLookup|VariantOverlapDetector|cached|disk_based"

# Find all Python files that import from variant_lookup
rg -t py "from prymer\.api\.variant_lookup import"

# Check for any references to the commented-out classes
rg -t py "_DiskBasedLookup|_InMemoryLookup|_VariantLookup"

Length of output: 6911

prymer/primer3/primer3.py (2)

232-239: Consider using more readable type hint syntax.

The Union type hint could be more readable.

-        variants: list[Path] | VariantLookup | None = None,
+        variants: None | list[Path] | VariantLookup = None,

---

250-270: Consider these improvements to variant lookup initialization:

1. Make default parameters configurable via constructor.
2. Type checking could be more robust.
3. Error message could be more specific.

-        elif isinstance(variants, list) and all(
-            isinstance(path, Path) for path in variants
-        ):
+        elif isinstance(variants, list) and all(isinstance(path, (Path, str)) for path in variants):
             self.variant_lookup = VariantLookup(
                 vcf_paths=variants,
-                cached=True,
-                min_maf=0.00,
-                include_missing_mafs=False,
+                cached=cache_variants,
+                min_maf=min_maf,
+                include_missing_mafs=include_missing_mafs,
             )
         else:
             raise ValueError(
-                "Variant lookup is required to be a list of `Path` objects or a"
-                f" pre-constructed `VariantLookup` object: received {variants}"
+                "Variant lookup must be a list of Path objects or a"
+                f" VariantLookup object, got {type(variants).__name__}"
             )

prymer/api/variant_lookup.py (6)

11-11: Typo in documentation: Duplicate word.

Correct "a minimum Minimum Allele Frequency" to "a minimum Allele Frequency".

---

12-13: Formatting issue in parameter descriptions.

Avoid starting a new sentence within the parameter list. Combine sentences for clarity.

---

371-379: Use public class in documentation example.

The example uses the private class _DiskBasedLookup. Update it to use the public VariantLookup class for better clarity.

---

250-253: Clarify the necessity of the close method in abstract base class.

Since not all subclasses of _VariantLookup require resource cleanup, consider whether close needs to be an abstract method.

---

310-313: Provide default values for constructor parameters.

In VariantLookup.__init__, consider setting default values for min_maf and include_missing_mafs to enhance usability.

---

343-345: Optional parameters may be unnecessary.

The maf and include_missing_mafs parameters in VariantLookup.query default to None but are immediately assigned values from the instance if None. Consider simplifying by removing these parameters or setting defaults.

tests/api/test_variant_lookup.py (6)

426-426: Remove unused parameter sample_vcf

The parameter sample_vcf is not used in test_simple_variant_conversion.

Clean up the function signature:

-def test_simple_variant_conversion(vcf_path: Path, sample_vcf: list[VariantRecord]) -> None:
+def test_simple_variant_conversion(vcf_path: Path) -> None:

---

441-445: Consistent use of context managers

In lines 441-445, VariantLookup is used within a with statement, but elsewhere it's instantiated without one.

For consistency and proper resource management, always use VariantLookup within a context manager.

---

466-469: Duplicate exception handling

Both context-managed and direct instantiation tests check for the same exception.

Combine them to avoid redundancy.

---

475-478: Redundant tests for missing VCF paths

Lines 475-478 repeat the same test in different contexts.

Simplify the tests by combining them.

---

572-577: Clarify cached parameter usage

In line 577, cached=True is set, but the test focus is on MAF filtering.

Ensure that caching is intended here, or adjust if unnecessary.

---

604-607: Consistent resource management

Lines 604-607 use a context manager but have commented-out code checking the lookup type.

Remove the commented code or update it if type checking is required.

tests/primer3/test_primer3.py (2)

57-62: Address the FIXME comment about variant parsing

The FIXME indicates that variant parsing could be improved to handle alternates more accurately. Enhancing this logic would improve the test coverage and accuracy.

Would you like assistance in implementing this improvement or opening a GitHub issue to track it?

---

411-472: Refactor tests to eliminate code duplication

The tests test_cached_variant_lookup and test_diskbased_variant_lookup share similar logic. Consider refactoring to reduce duplication, possibly by parameterizing over the cached flag.

Configuration used: CodeRabbit UI
Review profile: CHILL

lookup_cls = _InMemoryLookup if cached else _DiskBasedLookup
self._lookup = lookup_cls(
    vcf_paths=vcf_paths, min_maf=min_maf, include_missing_mafs=include_missing_mafs
)

prymer/api/variant_lookup.py (4)

10-15: Enhance documentation about memory usage implications.

Add explicit memory usage implications for the cached parameter.

 boolean `cached` flag. It is recommended to set `cached` to `False` for large VCFs. When `cached`
-is True, variants from smaller VCFs are loaded into memory.
+is True, variants from smaller VCFs are loaded into memory, which may significantly increase memory usage.
+When `cached` is False, variants are read from disk on-demand, reducing memory footprint but potentially
+impacting query performance.

---

Line range hint 256-275: Add return type documentation.

The query method's docstring is missing return type documentation.

         Args:
             refname: the reference name
             start: the 1-based start position
             end: the 1-based end position
             maf: the MAF of the variant
             include_missing_mafs: whether to include variants with a missing MAF
                 (overrides self.include_missing_mafs)
+        Returns:
+            A list of SimpleVariant objects that overlap the specified region.

---

359-366: Move MAF filtering to base class.

MAF filtering logic should be in _VariantLookup to avoid duplication in concrete implementations.

-        if len(variants) == 0:
-            _logger.debug(f"No variants extracted from region of interest: {refname}:{start}-{end}")
-        if maf is None or maf <= 0.0:
-            return variants
-        elif include_missing_mafs:  # return variants with a MAF above threshold or missing
-            return [v for v in variants if (v.maf is None or v.maf >= maf)]
-        else:
-            return [v for v in variants if v.maf is not None and v.maf >= maf]
+        return self._filter_by_maf(variants, maf, include_missing_mafs)

Add to _VariantLookup:

def _filter_by_maf(
    self,
    variants: list[SimpleVariant],
    maf: Optional[float],
    include_missing_mafs: bool,
) -> list[SimpleVariant]:
    if len(variants) == 0:
        _logger.debug("No variants extracted from region of interest")
    if maf is None or maf <= 0.0:
        return variants
    elif include_missing_mafs:
        return [v for v in variants if (v.maf is None or v.maf >= maf)]
    else:
        return [v for v in variants if v.maf is not None and v.maf >= maf]

---

Line range hint 392-403: Extract common validation logic.

Index file validation is duplicated. Move to base class.

+    @staticmethod
+    def _validate_vcf_paths(vcf_paths: list[Path]) -> None:
+        if len(vcf_paths) == 0:
+            raise ValueError("No VCF paths given to query.")
+        for path in vcf_paths:
+            if (not path.with_suffix(path.suffix + ".csi").is_file()
+                and not path.with_suffix(path.suffix + ".tbi").is_file()):
+                raise ValueError(f"Cannot perform fetch with missing index file for VCF: {path}")

Update both classes to use this method in their __init__.

Also applies to: 452-463

tests/api/test_variant_lookup.py (3)

Line range hint 497-502: Replace _DiskBasedLookup with VariantLookup

Direct usage of internal class. Use public interface.

-with _DiskBasedLookup(
+with VariantLookup(
     vcf_paths=vcf_paths,
     min_maf=0.00,
-    include_missing_mafs=True
+    include_missing_mafs=True,
+    cached=False
) as variant_lookup:

---

572-577: Update docstring to reflect VariantLookup usage

Docstring references internal class.

-"""Test that `_InMemoryLookup.query()` MAF filtering is as expected.
+"""Test that `VariantLookup.query()` MAF filtering is as expected.

---

607-607: Remove commented assertion

Dead code should be removed.

-        # assert isinstance(file_based_vcf_query._lookup, _DiskBasedLookup)

tests/primer3/test_primer3.py (3)

40-64: Add type hints to fixture docstring

Add return type hint to the docstring for better code documentation.

 @pytest.fixture()
 def variant_masking_test_data() -> list[tuple[Span, str, str]]:
+    """Return test data for variant masking.
+    
+    Returns:
+        list[tuple[Span, str, str]]: List of tuples containing:
+            - Span: Region to test
+            - str: Expected hard-masked sequence
+            - str: Expected soft-masked sequence
+    """
     return [

---

411-478: Add edge cases to variant lookup tests

Add tests for:

1. Empty VCF list
2. Non-existent VCF file
3. Malformed VCF file

def test_empty_vcf_list(genome_ref: Path) -> None:
    """Test behavior with empty VCF list."""
    with pytest.raises(ValueError, match="At least one VCF file required"):
        Primer3(genome_fasta=genome_ref, variants=[])

def test_nonexistent_vcf(genome_ref: Path) -> None:
    """Test behavior with non-existent VCF."""
    with pytest.raises(FileNotFoundError):
        Primer3(genome_fasta=genome_ref, variants=[Path("nonexistent.vcf")])

---

485-531: Add boundary test cases for design region

Add test cases for:

1. Minimum valid amplicon length
2. Maximum supported amplicon length
3. Equal target and amplicon lengths

@pytest.mark.parametrize("amplicon_length", [
    20,  # minimum (2 * min_primer_length)
    1000000,  # maximum supported
    50,  # equal to target length
])
def test_create_design_region_boundaries(amplicon_length: int, genome_ref: Path) -> None:
    """Test boundary conditions for design region creation."""
    target_region = Span(refname="chr1", start=201, end=250)
    with Primer3(genome_fasta=genome_ref) as designer:
        if amplicon_length < target_region.length:
            with pytest.raises(ValueError, match="exceeds the maximum size"):
                designer._create_design_region(
                    target_region=target_region,
                    max_amplicon_length=amplicon_length,
                    min_primer_length=10,
                )
        else:
            design_region = designer._create_design_region(
                target_region=target_region,
                max_amplicon_length=amplicon_length,
                min_primer_length=10,
            )
            assert design_region.length >= target_region.length

prymer/primer3/primer3.py (2)

250-265: Consider using a dataclass for variant parameters.

The default parameters (cached, min_maf, include_missing_mafs) could be encapsulated in a dataclass for better maintainability.

+@dataclass
+class VariantParams:
+    cached: bool = True
+    min_maf: float = 0.00
+    include_missing_mafs: bool = False

 def __init__(
     self,
     genome_fasta: Path,
     executable: Optional[str] = None,
     variants: list[Path] | VariantLookup | None = None,
+    variant_params: Optional[VariantParams] = None,
 ) -> None:

---

266-270: Improve error message specificity.

The error message could be more specific about the actual type received.

-            raise ValueError(
-                "Variant lookup is required to be a list of `Path` objects or a"
-                f" pre-constructed `VariantLookup` object: received {variants}"
-            )
+            raise ValueError(
+                f"Variant lookup must be a list of Path objects or a VariantLookup object, "
+                f"got {type(variants).__name__}"
+            )

Configuration used: CodeRabbit UI
Review profile: CHILL

-detector: VariantLookup | _VariantLookup,
+detector: VariantLookup,

-variant_lookup = VariantLookup(
+with VariantLookup(
     vcf_paths=[vcf_path], min_maf=0.01, include_missing_mafs=False, cached=False
-)
+) as variant_lookup:
     variant_lookup.query(refname="chr2", start=1, end=2)

-variant_overlap_detector = _InMemoryLookup(
+variant_overlap_detector = VariantLookup(
     vcf_paths=[vcf_path],
     min_maf=0.0,
-    include_missing_mafs=True
+    include_missing_mafs=True,
+    cached=True
)

prymer/api/variant_lookup.py (3)

4-14: Add type hints to examples.

Add type hints to improve code readability in documentation.

->>> lookup = VariantLookup(vcf_paths=[Path("./tests/api/data/miniref.variants.vcf.gz")], min_maf=0.00, include_missing_mafs=True)
+>>> lookup: VariantLookup = VariantLookup(vcf_paths=[Path("./tests/api/data/miniref.variants.vcf.gz")], min_maf=0.00, include_missing_mafs=True)

---

251-254: Add docstring to abstract close method.

Document the purpose of the close method in the abstract class.

 @abstractmethod
 def close(self) -> None:
+    """Close any resources held by this lookup implementation."""
     pass

---

Line range hint 391-401: Extract duplicate index validation logic.

Move VCF index validation to a common method.

+def _validate_vcf_index(path: Path) -> None:
+    """Validate that the VCF file has an index."""
+    if (
+        not path.with_suffix(path.suffix + ".csi").is_file()
+        and not path.with_suffix(path.suffix + ".tbi").is_file()
+    ):
+        raise ValueError(f"Cannot perform fetch with missing index file for VCF: {path}")

 class _DiskBasedLookup(_VariantLookup):
     def __init__(self, vcf_paths: list[Path], min_maf: Optional[float], include_missing_mafs: bool):
         self._readers: list[VariantFile] = []
         super().__init__(
             vcf_paths=vcf_paths, min_maf=min_maf, include_missing_mafs=include_missing_mafs
         )
         if len(vcf_paths) == 0:
             raise ValueError("No VCF paths given to query.")
         for path in vcf_paths:
-            if (
-                not path.with_suffix(path.suffix + ".csi").is_file()
-                and not path.with_suffix(path.suffix + ".tbi").is_file()
-            ):
-                raise ValueError(f"Cannot perform fetch with missing index file for VCF: {path}")
+            _validate_vcf_index(path)

Apply the same change in _InMemoryLookup.__init__.

Also applies to: 456-466

tests/api/test_variant_lookup.py (3)

Line range hint 496-501: Replace _DiskBasedLookup with VariantLookup.

Direct usage of internal implementation class.

-    with _DiskBasedLookup(
+    with VariantLookup(
         vcf_paths=vcf_paths, min_maf=0.00, include_missing_mafs=True
     ) as variant_lookup:

---

571-574: Update docstring to reflect new class name.

Docstring still references old internal class name.

-    """Test that `_InMemoryLookup.query()` MAF filtering is as expected.
+    """Test that `VariantLookup.query()` MAF filtering is as expected.

---

606-606: Remove commented assertion.

Dead code should be removed.

-        # assert isinstance(file_based_vcf_query._lookup, _DiskBasedLookup)

Configuration used: CodeRabbit UI
Review profile: CHILL

-    variant_lookup = VariantLookup(
-        vcf_paths=[vcf_path], min_maf=0.01, include_missing_mafs=False, cached=False
-    )
-    variant_lookup.query(refname="chr2", start=1, end=2)
+    with VariantLookup(
+        vcf_paths=[vcf_path], min_maf=0.01, include_missing_mafs=False, cached=False
+    ) as variant_lookup:
+        variant_lookup.query(refname="chr2", start=1, end=2)

-    variant_overlap_detector = _InMemoryLookup(
+    variant_overlap_detector = VariantLookup(
         vcf_paths=[vcf_path], min_maf=0.0, include_missing_mafs=True, cached=True
     )

-from prymer.api.variant_lookup import _DiskBasedLookup
-from prymer.api.variant_lookup import _InMemoryLookup