From 53ebeacbd207b0c64c787398ead4edb049be39a1 Mon Sep 17 00:00:00 2001 From: Sanjay Nagi Date: Thu, 13 Aug 2026 21:36:01 +0000 Subject: [PATCH] fix: unreachable chromid warning, shared mutable defaults, unclosed bed files Three unrelated correctness issues found while profiling. 1. The 'Multiple contigs above the specified chromosome length -c' warning could never fire. c is an int (c = 1; c += 1) but was compared against the string "2", so users assembling something with two chromosome-sized contigs - chromids, or a -c set too low - silently got no hint. Three copies, in the raven, flye and flye_long variants. 2. Mutable and import-time-evaluated default arguments in Plass.__init__ and Assembly.__init__: 'filtered_out_contig_ids: list = []', 'plasmid_names: list = ["1"]', and three 'depth_df: pd.DataFrame() = pd.DataFrame({...})' defaults. The list defaults are shared by every instance; the annotation 'pd.DataFrame()' also constructs a throwaway DataFrame at import time, six times, and the default value itself was a dummy {'col1': [1,2,3]} frame rather than an empty one. All now default to None and are materialised per instance. Assembly.__init__ also accepted chromosome_name and plasmid_names without ever storing them; it does now. 3. non_chromosome.bed and chromosome.bed were opened with a bare open() and never closed. samtools later reads both with -L, which worked only because CPython's refcounting flushed them when the method returned. Closed explicitly. New tests/test_plass_class_defaults.py covers each: instances no longer share default containers, supplied values still win, the DataFrame defaults are empty, and the multiple-chromosome warning actually fires. --- src/plassembler/utils/plass_class.py | 72 +++++++++++++++++++--------- tests/test_plass_class_defaults.py | 64 +++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 22 deletions(-) create mode 100644 tests/test_plass_class_defaults.py diff --git a/src/plassembler/utils/plass_class.py b/src/plassembler/utils/plass_class.py index b8e7318..b730940 100644 --- a/src/plassembler/utils/plass_class.py +++ b/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 diff --git a/tests/test_plass_class_defaults.py b/tests/test_plass_class_defaults.py new file mode 100644 index 0000000..3f4d03c --- /dev/null +++ b/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