fix: unreachable chromid warning, shared mutable defaults, unclosed bed file handles

src/plassembler/utils/plass_class.py

@@ -1,5 +1,6 @@
 import os
 from pathlib import Path
+from typing import Optional
 
 import pandas as pd
 from Bio import SeqIO
@@ -30,14 +31,12 @@ def __init__(
         no_plasmids_flag: bool = False,
         chromosome_flag: bool = True,
         threads: int = 1,
-        depth_df: pd.DataFrame() = pd.DataFrame({"col1": [1, 2, 3], "col2": [4, 5, 6]}),
-        mash_df: pd.DataFrame() = pd.DataFrame({"col1": [1, 2, 3], "col2": [4, 5, 6]}),
-        combined_depth_mash_df: pd.DataFrame() = pd.DataFrame(
-            {"col1": [1, 2, 3], "col2": [4, 5, 6]}
-        ),
+        depth_df: Optional[pd.DataFrame] = None,
+        mash_df: Optional[pd.DataFrame] = None,
+        combined_depth_mash_df: Optional[pd.DataFrame] = None,
         long_only: bool = False,
         unicycler_success: bool = True,
-        filtered_out_contig_ids: list = [],
+        filtered_out_contig_ids: Optional[list] = None,
     ) -> None:
         """
         Parameters
@@ -68,12 +67,18 @@ def __init__(
         self.no_plasmids_flag = no_plasmids_flag
         self.chromosome_flag = chromosome_flag
         self.threads = threads
-        self.depth_df = depth_df
-        self.mash_df = mash_df
-        self.combined_depth_mash_df = combined_depth_mash_df
+        # None rather than a shared default instance: a mutable default is
+        # created once at import and shared by every instance
+        self.depth_df = pd.DataFrame() if depth_df is None else depth_df
+        self.mash_df = pd.DataFrame() if mash_df is None else mash_df
+        self.combined_depth_mash_df = (
+            pd.DataFrame() if combined_depth_mash_df is None else combined_depth_mash_df
+        )
         self.long_only = long_only
         self.unicycler_success = unicycler_success
-        self.filtered_out_contig_ids = filtered_out_contig_ids
+        self.filtered_out_contig_ids = (
+            [] if filtered_out_contig_ids is None else filtered_out_contig_ids
+        )
 
     def get_contig_count(self):
         """Counts the number of contigs assembled
@@ -135,7 +140,7 @@ def identify_chromosome_process_raven(self, chromosome_len):
                             if c == 1:
                                 dna_header = "chromosome"
                             else:
-                                if c == "2":
+                                if c == 2:
                                     message = "Multiple contigs above the specified chromosome length -c have been detected. \nIf you are hoping for plasmids from haploid bacteria, please check your value for -c."
                                     logger.info(message)
                                 dna_header = "chromosome_" + str(c)
@@ -200,6 +205,12 @@ def identify_chromosome_process_raven(self, chromosome_len):
                                 f"{dna_header}\t1\t{contig_len}\n"
                             )  # Write read name
                             i += 1
+            # these were opened without a context manager, so close them
+            # explicitly: downstream samtools reads them with -L, and until
+            # now that only worked because CPython's refcounting happened to
+            # flush them when this method returned
+            bed_file.close()
+            bed_chrom_file.close()
         # add to object
         self.chromosome_flag = chromosome_flag
 
@@ -256,7 +267,7 @@ def identify_chromosome_process_flye(self, chromosome_len):
                             if c == 1:
                                 dna_header = "chromosome"
                             else:
-                                if c == "2":
+                                if c == 2:
                                     message = "Multiple contigs above the specified chromosome length -c have been detected. \nIf you are hoping for plasmids from haploid bacteria, please check your value for -c."
                                     logger.info(message)
                                 dna_header = "chromosome_" + str(c)
@@ -291,6 +302,12 @@ def identify_chromosome_process_flye(self, chromosome_len):
                             )  # Write read name
                             i += 1
 
+            # these were opened without a context manager, so close them
+            # explicitly: downstream samtools reads them with -L, and until
+            # now that only worked because CPython's refcounting happened to
+            # flush them when this method returned
+            bed_file.close()
+            bed_chrom_file.close()
         # add to object
         self.chromosome_flag = chromosome_flag
 
@@ -347,7 +364,7 @@ def identify_chromosome_process_flye_long(self, chromosome_len):
                             if c == 1:
                                 dna_header = "chromosome"
                             else:
-                                if c == "2":
+                                if c == 2:
                                     message = "Multiple contigs above the specified chromosome length -c have been detected. \nIf you are hoping for plasmids from haploid bacteria, please check your value for -c."
                                     logger.info(message)
                                 dna_header = "chromosome_" + str(c)
@@ -380,6 +397,12 @@ def identify_chromosome_process_flye_long(self, chromosome_len):
                                 f"{dna_header}\t1\t{contig_len}\n"
                             )  # Write read name
                             i += 1
+            # these were opened without a context manager, so close them
+            # explicitly: downstream samtools reads them with -L, and until
+            # now that only worked because CPython's refcounting happened to
+            # flush them when this method returned
+            bed_file.close()
+            bed_chrom_file.close()
         # add to object
         self.chromosome_flag = chromosome_flag
 
@@ -884,13 +907,11 @@ def __init__(
         short_flag: bool = True,
         chromosome_name: str = "chromosome",
         contig_count: int = 1,
-        plasmid_names: list = ["1"],
+        plasmid_names: Optional[list] = None,
         threads: int = 1,
-        depth_df: pd.DataFrame() = pd.DataFrame({"col1": [1, 2, 3], "col2": [4, 5, 6]}),
-        mash_df: pd.DataFrame() = pd.DataFrame({"col1": [1, 2, 3], "col2": [4, 5, 6]}),
-        combined_depth_mash_df: pd.DataFrame() = pd.DataFrame(
-            {"col1": [1, 2, 3], "col2": [4, 5, 6]}
-        ),
+        depth_df: Optional[pd.DataFrame] = None,
+        mash_df: Optional[pd.DataFrame] = None,
+        combined_depth_mash_df: Optional[pd.DataFrame] = None,
     ) -> None:
         """
         Parameters
@@ -913,11 +934,18 @@ def __init__(
         self.outdir = outdir
         self.contig_count = contig_count
         self.threads = threads
-        self.depth_df = depth_df
-        self.mash_df = mash_df
-        self.combined_depth_mash_df = combined_depth_mash_df
+        # None rather than a shared default instance: a mutable default is
+        # created once at import and shared by every instance
+        self.depth_df = pd.DataFrame() if depth_df is None else depth_df
+        self.mash_df = pd.DataFrame() if mash_df is None else mash_df
+        self.combined_depth_mash_df = (
+            pd.DataFrame() if combined_depth_mash_df is None else combined_depth_mash_df
+        )
         self.long_flag = long_flag
         self.short_flag = short_flag
+        self.chromosome_name = chromosome_name
+        # None rather than a shared default list, which a mutable default would be
+        self.plasmid_names = ["1"] if plasmid_names is None else plasmid_names
 
     def combine_input_fastas(self, chromosome_fasta: Path, plasmids_fasta: Path):
         """wrapper function to get depth of each plasmid

tests/test_plass_class_defaults.py

@@ -0,0 +1,64 @@
+"""Regression tests for constructor defaults and the chromid warning."""
+
+import pandas as pd
+
+from src.plassembler.utils.plass_class import Assembly, Plass
+
+
+def test_plass_mutable_defaults_are_not_shared():
+    """A mutable default is created once at import and shared by every instance."""
+    first, second = Plass(), Plass()
+    assert first.filtered_out_contig_ids is not second.filtered_out_contig_ids
+    first.filtered_out_contig_ids.append("1")
+    assert second.filtered_out_contig_ids == []
+
+
+def test_assembly_mutable_defaults_are_not_shared():
+    first, second = Assembly(), Assembly()
+    assert first.plasmid_names is not second.plasmid_names
+    first.plasmid_names.append("2")
+    assert second.plasmid_names == ["1"]
+
+
+def test_dataframe_defaults_are_empty_and_per_instance():
+    """The defaults used to be a dummy {'col1': [1,2,3]} frame built at import."""
+    first, second = Plass(), Plass()
+    for frame in (first.depth_df, first.mash_df, first.combined_depth_mash_df):
+        assert isinstance(frame, pd.DataFrame)
+        assert frame.empty
+    assert first.depth_df is not second.depth_df
+
+
+def test_supplied_values_are_still_used():
+    df = pd.DataFrame({"contig": ["1"]})
+    plass = Plass(depth_df=df, filtered_out_contig_ids=["7"])
+    assert plass.depth_df is df
+    assert plass.filtered_out_contig_ids == ["7"]
+
+
+def test_multiple_chromosome_warning_can_fire(tmp_path, caplog):
+    """`c` is an int, so the old `if c == "2"` could never be true and the
+    "multiple contigs above -c" warning was unreachable."""
+    from loguru import logger
+
+    assembly = tmp_path / "assembly.fasta"
+    # two contigs above the 1000 bp chromosome threshold, plus one below
+    assembly.write_text(
+        ">contig_1\n" + "A" * 2000 + "\n"
+        ">contig_2\n" + "C" * 1500 + "\n"
+        ">contig_3\n" + "G" * 100 + "\n"
+    )
+
+    messages = []
+    sink_id = logger.add(lambda m: messages.append(str(m)), level="INFO")
+    try:
+        plass = Plass()
+        plass.outdir = str(tmp_path)
+        plass.identify_chromosome_process_raven(1000)
+    finally:
+        logger.remove(sink_id)
+
+    assert any(
+        "Multiple contigs above the specified chromosome length" in m for m in messages
+    )
+    assert plass.chromosome_flag is True