From 1bdd3cbcb17e2c918a6160375f769e56520c8529 Mon Sep 17 00:00:00 2001 From: isayev Date: Mon, 3 Aug 2026 17:06:20 -0400 Subject: [PATCH] refactor!: delete the dead code M53 was right about, after checking all of it BREAKING CHANGE: `Auto3D.utils_file` is removed, along with `Auto3D.utils.encode_smiles`, `decode_smiles` and `housekeeping_helper` (which were in `utils/__init__.__all__`), `cli.results.count_from_output`, and the `BOND_STRETCH_TOLERANCE`, `COLLISION_THRESHOLD` and `SUPPORTED_MODELS` constants. None had a production caller. Per the release decision, deleted outright rather than shimmed. Re-verifying M53 before acting on it was the whole point, and it was justified: only 4 of its 13 entries described code that was both present and dead. - Three were already deleted by earlier phases: pad_molecular_batch, create_progress, IsomerProgressCallback -- zero hits in src/. - Six are not dead. utils/stereo_check has six live uses; ASE/thermo's mol2atoms and STANDARD_PRESSURE were revived by this session's own thermo work; isomers/parallel_embed was wired to Auto3DOptions in the previous change; and cli/results' FailedMolecule and print_failures are called from run.py, so the finding's supporting claim -- "run.py:149 admits failures is always []" -- stopped being true when the C6/C7 reconciliation landed. - Three cannot be resolved from the finding at all: its line numbers are stale. exceptions.py:41 is OptimizationError, which is raised three times, so the cited lines no longer point at the four never-raised classes described. Those, the model_wrapper legacy `name` API (still warning "removed in Auto3D v2.0" at version 3.0.0) and ASE/thermo's unread model_name param need their own pass. Net 256 lines out of src/ and 167 out of tests/, against a claimed ~450 in src/ -- the gap is a third already gone and half of the remainder alive. Every deleted symbol's tests went with it, which is why this ran before the test-hardening step: four test classes and two test functions are gone rather than being improved. Verified: 1273 passed, 9 skipped; ruff clean. --- .../follow-ups-after-4.0.0-remediation.md | 20 ++- src/Auto3D/cli/results.py | 12 -- src/Auto3D/constants.py | 5 - src/Auto3D/utils/__init__.py | 6 - src/Auto3D/utils/file_ops.py | 148 ----------------- src/Auto3D/utils_file.py | 85 ---------- tests/test_cli_results.py | 16 -- tests/test_utils_file_ops.py | 151 ------------------ 8 files changed, 16 insertions(+), 427 deletions(-) delete mode 100644 src/Auto3D/utils_file.py diff --git a/docs/superpowers/follow-ups-after-4.0.0-remediation.md b/docs/superpowers/follow-ups-after-4.0.0-remediation.md index 640a378b..df633729 100644 --- a/docs/superpowers/follow-ups-after-4.0.0-remediation.md +++ b/docs/superpowers/follow-ups-after-4.0.0-remediation.md @@ -112,10 +112,22 @@ Verified against current source: | `constants.py` `check_connectivity` hardcodes 1.25/1.1 | recheck; the surrounding code has moved | | `isomers/parallel_embed.py` 138 lines | claim stands, but see the decision needed above | -The remaining nine entries were not re-checked and must be before anything is -deleted. This is the third time a manifest entry has turned out to be already -closed or wrong; the pattern is now reliable enough to treat every entry as a -claim rather than a fact. +**Full re-verification, 2026-08-03.** All 13 entries checked against source. The +list was **substantially wrong** — only 4 of 13 described code that was both +present and dead: + +| verdict | entries | +|---|---| +| **already deleted** by earlier phases | `pad_molecular_batch`, `cli/progress` `create_progress`, `IsomerProgressCallback` | +| **not dead — M53 wrong** | `utils/stereo_check` (6 live uses), `cli/results` `FailedMolecule` + `print_failures` (live from `run.py` since the C6/C7 reconciliation work — the finding's "run.py admits failures is always []" no longer holds), `ASE/thermo` `mol2atoms`, `STANDARD_PRESSURE`, `isomers/parallel_embed` | +| **genuinely dead — deleted** | `utils_file.py` (whole module), `count_from_output`, `encode_smiles`, `decode_smiles`, `housekeeping_helper`, and 3 constants (`BOND_STRETCH_TOLERANCE`, `COLLISION_THRESHOLD`, `SUPPORTED_MODELS`) | +| **unresolved — line numbers stale** | `exceptions.py` "4 classes never raised" (line 41 is `OptimizationError`, raised 3x, so the cited lines no longer point at what the finding describes); `model_wrapper`'s legacy `name` API; `ASE/thermo` `model_name` param | + +Net: **256 lines removed from `src/`, 167 from `tests/`** — not the ~450 of `src/` +the finding claimed, because a third of it was gone and half of the rest is alive. + +The three unresolved entries need their own pass; the audit's line numbers cannot +be used to find them. ## What the remediation closed diff --git a/src/Auto3D/cli/results.py b/src/Auto3D/cli/results.py index 4126877e..ffd6f36f 100644 --- a/src/Auto3D/cli/results.py +++ b/src/Auto3D/cli/results.py @@ -105,18 +105,6 @@ def print_failures(failures: list[FailedMolecule], verbose: bool = False) -> Non console.print("[dim]Run with -v to see details[/dim]") -def count_from_output(output_path: str) -> tuple[int, int]: - """Return (unique_molecule_count, conformer_count) from an output SDF. - - Thin back-compat wrapper around :func:`Auto3D.results.count_output` (the - single source of truth). ``main()`` now returns a ``WorkflowResult`` that - carries these counts, so the CLI reads them off the result instead. - """ - from Auto3D.results import count_output - - return count_output(output_path) - - def output_json(results: WorkflowResults) -> None: """Output results as JSON. diff --git a/src/Auto3D/constants.py b/src/Auto3D/constants.py index 28eef219..1b808aae 100644 --- a/src/Auto3D/constants.py +++ b/src/Auto3D/constants.py @@ -7,8 +7,6 @@ HARTREE_TO_KCAL_PER_MOL = 627.50947337481 # 1 Hartree in kcal/mol # Geometry thresholds -BOND_STRETCH_TOLERANCE = 1.25 # Maximum bond stretch factor -COLLISION_THRESHOLD = 1.1 # Å, minimum distance for clash detection MIN_ATOM_DISTANCE = 0.9 # Å, minimum allowed interatomic distance # Conformer generation limits @@ -43,9 +41,6 @@ MODEL_ANI2XT = "ANI2xt" # Supported model names (for validation) -SUPPORTED_MODELS = frozenset({MODEL_AIMNET, MODEL_ANI2X, MODEL_ANI2XT}) - -# Backward-compatible alias: "AIMNET" now maps to the aimnet registry default. DEFAULT_AIMNET_MODEL = "aimnet2" # Built-in (non-aimnet) engines kept for back-compat. BUILTIN_ANI_MODELS = frozenset({MODEL_ANI2X.upper(), MODEL_ANI2XT.upper()}) diff --git a/src/Auto3D/utils/__init__.py b/src/Auto3D/utils/__init__.py index 758c7b1b..32f34f19 100644 --- a/src/Auto3D/utils/__init__.py +++ b/src/Auto3D/utils/__init__.py @@ -30,14 +30,11 @@ combine_smi, create_chunk_meta_names, decode_ids, - decode_smiles, encode_ids, - encode_smiles, guess_file_type, hash_enumerated_smi_IDs, hash_taut_smi, housekeeping, - housekeeping_helper, reorder_sdf, ) from Auto3D.utils.logging_config import configure_logging, get_logger @@ -99,11 +96,8 @@ "check_valid_configuration", # File operations "guess_file_type", - "encode_smiles", - "decode_smiles", "hash_enumerated_smi_IDs", "hash_taut_smi", - "housekeeping_helper", "housekeeping", "create_chunk_meta_names", "combine_smi", diff --git a/src/Auto3D/utils/file_ops.py b/src/Auto3D/utils/file_ops.py index 48552d17..b1ed1be7 100644 --- a/src/Auto3D/utils/file_ops.py +++ b/src/Auto3D/utils/file_ops.py @@ -12,9 +12,7 @@ """ from __future__ import annotations -import base64 import collections -import hashlib import os import shutil from collections import defaultdict @@ -176,135 +174,6 @@ def guess_file_type(filename: str) -> str: return Path(filename).suffix[1:] -def encode_smiles(smiles: str, max_length: int = 50) -> str: - """Encode a SMILES string for use in filenames. - - Transforms a SMILES string into a filesystem-safe string by replacing - special characters with alphanumeric equivalents. For SMILES longer - than max_length, uses a hash-based encoding. - - The encoding maps common SMILES characters to filename-safe alternatives: - - '=' -> 'd' (double bond) - - '#' -> 't' (triple bond) - - '@' -> 'a' (stereochemistry) - - '/' -> 's' (cis/trans) - - '\\' -> 'b' (cis/trans) - - '+' -> 'p' (positive charge) - - '-' -> 'm' (negative charge) - - '(' -> 'L' (left paren) - - ')' -> 'R' (right paren) - - '[' -> 'K' (left bracket) - - ']' -> 'J' (right bracket) - - '%' -> 'X' (ring number indicator) - - Args: - smiles: The SMILES string to encode. - max_length: Maximum length for encoded string before using hash. - Defaults to 50 characters. - - Returns: - A filesystem-safe encoded string representing the SMILES. - - Example: - >>> encode_smiles("CCO") - 'CCO' - >>> encode_smiles("C=C") - 'CdC' - >>> encode_smiles("C#N") - 'CtN' - >>> encode_smiles("[NH4+]") - 'KNH4pJ' - """ - # Define character replacements for filesystem safety - # Using single lowercase letters that are unlikely to cause collisions - replacements = { - '=': 'd', # double bond - '#': 't', # triple bond - '@': 'a', # stereochemistry - '/': 's', # cis/trans - '\\': 'b', # cis/trans (backslash) - '+': 'p', # positive charge - '-': 'm', # negative charge (also single bond, but rare in SMILES) - '(': 'L', # left parenthesis - ')': 'R', # right parenthesis - '[': 'K', # left bracket - ']': 'J', # right bracket - '%': 'X', # ring number indicator (for rings > 9) - } - - # Apply replacements - encoded = smiles - for char, replacement in replacements.items(): - encoded = encoded.replace(char, replacement) - - # If still too long, use a hash-based encoding - if len(encoded) > max_length: - # Use SHA256 hash truncated to produce a shorter, unique identifier - hash_obj = hashlib.sha256(smiles.encode('utf-8')) - # Take first 16 characters of base64-encoded hash (url-safe) - hash_str = base64.urlsafe_b64encode(hash_obj.digest()[:12]).decode('utf-8') - # Combine a prefix of the encoded SMILES with the hash - prefix_len = max_length - len(hash_str) - 1 # -1 for separator - if prefix_len > 0: - encoded = f"{encoded[:prefix_len]}_{hash_str}" - else: - encoded = hash_str - - return encoded - - -def decode_smiles(encoded: str) -> str: - """Decode an encoded SMILES string back to the original SMILES. - - Reverses the encoding performed by encode_smiles for short SMILES strings. - Note: For hash-encoded (long) SMILES, the original cannot be recovered. - - Args: - encoded: The encoded SMILES string. - - Returns: - The decoded SMILES string. For hash-encoded strings, returns the - input unchanged since the original cannot be recovered. - - Example: - >>> decode_smiles("CdC") - 'C=C' - >>> decode_smiles("CtN") - 'C#N' - >>> decode_smiles("KNH4pJ") - '[NH4+]' - """ - # Define reverse replacements - # Order matters: longer replacements should not interfere with shorter ones - replacements = { - 'd': '=', # double bond - 't': '#', # triple bond - 'a': '@', # stereochemistry - 's': '/', # cis/trans - 'b': '\\', # cis/trans (backslash) - 'p': '+', # positive charge - 'm': '-', # negative charge - 'L': '(', # left parenthesis - 'R': ')', # right parenthesis - 'K': '[', # left bracket - 'J': ']', # right bracket - 'X': '%', # ring number indicator - } - - # Check if this looks like a hash-encoded string (contains underscore near end - # followed by base64-like characters) - if '_' in encoded and len(encoded.split('_')[-1]) >= 12: - # Likely hash-encoded, can't decode - return encoded - - # Apply reverse replacements - decoded = encoded - for char, replacement in replacements.items(): - decoded = decoded.replace(char, replacement) - - return decoded - - def hash_enumerated_smi_IDs(smi: str, out: str) -> None: """Write all SMILES with hashed IDs into a new file. @@ -368,23 +237,6 @@ def hash_taut_smi(smi: str, out: str) -> None: f.write(molecule) -def housekeeping_helper(folder: str, file: str) -> None: - """Move a file into the specified folder. - - Args: - folder: Destination folder path. - file: Path to the file to move. - - Returns: - None. Moves the file to the destination folder. - - Example: - >>> housekeeping_helper("/tmp/output", "/tmp/results.sdf") - """ - new_name = Path(folder) / Path(file).name - shutil.move(file, str(new_name)) - - def housekeeping(job_name: str, folder: str, optimized_structures: str) -> None: """Move this job directory's metadata files into a folder. diff --git a/src/Auto3D/utils_file.py b/src/Auto3D/utils_file.py deleted file mode 100644 index ebed0474..00000000 --- a/src/Auto3D/utils_file.py +++ /dev/null @@ -1,85 +0,0 @@ -#!/usr/bin/env python -""" -Providing general utilities for working with different formats of molecular files - -.. deprecated:: 3.0 - This module is deprecated. Use :mod:`Auto3D.utils.file_ops` instead. - Functions will be removed in Auto3D v4.0. -""" -from __future__ import annotations - -import warnings - -_DEPRECATION_MESSAGE = ( - "Auto3D.utils_file is deprecated and will be removed in Auto3D v4.0. " - "Import from Auto3D.utils.file_ops instead." -) - - -def _warn() -> None: - """Emit the module-level deprecation warning.""" - warnings.warn(_DEPRECATION_MESSAGE, DeprecationWarning, stacklevel=3) - - -def guess_file_type(*args, **kwargs): - """Deprecated. Delegates to :func:`Auto3D.utils.file_ops.guess_file_type`.""" - _warn() - from Auto3D.utils import file_ops - - return file_ops.guess_file_type(*args, **kwargs) - - -def smiles2smi(*args, **kwargs): - """Deprecated. Delegates to :func:`Auto3D.utils.file_ops.smiles2smi`.""" - _warn() - from Auto3D.utils import file_ops - - return file_ops.smiles2smi(*args, **kwargs) - - -def combine_smi(*args, **kwargs): - """Deprecated. Delegates to :func:`Auto3D.utils.file_ops.combine_smi`.""" - _warn() - from Auto3D.utils import file_ops - - return file_ops.combine_smi(*args, **kwargs) - - -def countSDF(*args, **kwargs): - """Deprecated. Delegates to :func:`Auto3D.utils.file_ops.count_sdf`.""" - _warn() - from Auto3D.utils import file_ops - - return file_ops.count_sdf(*args, **kwargs) - - -def SDF2chunks(*args, **kwargs): - """Deprecated. Delegates to :func:`Auto3D.utils.file_ops.SDF2chunks`.""" - _warn() - from Auto3D.utils import file_ops - - return file_ops.SDF2chunks(*args, **kwargs) - - -def find_smiles_not_in_sdf(*args, **kwargs): - """Deprecated. Delegates to :func:`Auto3D.utils.file_ops.find_smiles_not_in_sdf`.""" - _warn() - from Auto3D.utils import file_ops - - return file_ops.find_smiles_not_in_sdf(*args, **kwargs) - - -def encode_ids(*args, **kwargs): - """Deprecated. Delegates to :func:`Auto3D.utils.file_ops.encode_ids`.""" - _warn() - from Auto3D.utils import file_ops - - return file_ops.encode_ids(*args, **kwargs) - - -def decode_ids(*args, **kwargs): - """Deprecated. Delegates to :func:`Auto3D.utils.file_ops.decode_ids`.""" - _warn() - from Auto3D.utils import file_ops - - return file_ops.decode_ids(*args, **kwargs) diff --git a/tests/test_cli_results.py b/tests/test_cli_results.py index a48780a0..1d3d15cc 100644 --- a/tests/test_cli_results.py +++ b/tests/test_cli_results.py @@ -96,19 +96,3 @@ def test_output_json(): output_json(results) -def test_count_from_output_counts_molecules_and_conformers(tmp_path): - from rdkit import Chem - from rdkit.Chem import AllChem - from Auto3D.cli.results import count_from_output - - out = tmp_path / "out.sdf" - with Chem.SDWriter(str(out)) as w: - for name in ["a", "a", "b"]: # 2 unique ids, 3 conformers - m = Chem.AddHs(Chem.MolFromSmiles("CCO")) - AllChem.EmbedMolecule(m, randomSeed=1) - m.SetProp("_Name", name) - w.write(m) - - molecules, conformers = count_from_output(str(out)) - assert molecules == 2 - assert conformers == 3 diff --git a/tests/test_utils_file_ops.py b/tests/test_utils_file_ops.py index 353316ad..dbfdddc0 100644 --- a/tests/test_utils_file_ops.py +++ b/tests/test_utils_file_ops.py @@ -7,11 +7,8 @@ from Auto3D.utils.file_ops import ( smiles2smi, guess_file_type, - encode_smiles, - decode_smiles, hash_enumerated_smi_IDs, hash_taut_smi, - housekeeping_helper, housekeeping, create_chunk_meta_names, combine_smi, @@ -197,136 +194,6 @@ def test_hidden_file(self): assert guess_file_type(".hidden.sdf") == "sdf" -class TestEncodeSmiles: - """Tests for encode_smiles function.""" - - def test_simple_smiles(self): - """Test that simple SMILES without special chars are unchanged.""" - assert encode_smiles("CCO") == "CCO" - assert encode_smiles("CCCC") == "CCCC" - assert encode_smiles("c1ccccc1") == "c1ccccc1" - - def test_double_bond(self): - """Test encoding of double bonds.""" - assert encode_smiles("C=C") == "CdC" - assert encode_smiles("CC=CC") == "CCdCC" - - def test_triple_bond(self): - """Test encoding of triple bonds.""" - assert encode_smiles("C#N") == "CtN" - assert encode_smiles("C#C") == "CtC" - - def test_stereochemistry(self): - """Test encoding of stereochemistry markers.""" - assert encode_smiles("C/C=C/C") == "CsCdCsC" - assert encode_smiles("C/C=C\\C") == "CsCdCbC" - - def test_chiral_center(self): - """Test encoding of chiral centers.""" - encoded = encode_smiles("[C@H](F)(Cl)Br") - assert "a" in encoded # @ becomes 'a' - assert "K" in encoded # [ becomes 'K' - assert "J" in encoded # ] becomes 'J' - - def test_charged_species(self): - """Test encoding of charged molecules.""" - encoded = encode_smiles("[NH4+]") - assert encoded == "KNH4pJ" - - encoded = encode_smiles("[O-]") - assert encoded == "KOmJ" - - def test_parentheses(self): - """Test encoding of parentheses.""" - encoded = encode_smiles("CC(C)C") - assert encoded == "CCLCRC" - - def test_brackets(self): - """Test encoding of brackets.""" - encoded = encode_smiles("[Na]") - assert encoded == "KNaJ" - - def test_ring_numbers_with_percent(self): - """Test encoding of large ring numbers.""" - encoded = encode_smiles("C%12CCCCC%12") - assert "X12" in encoded - - def test_long_smiles_uses_hash(self): - """Test that very long SMILES are hash-encoded.""" - # Create a SMILES longer than 50 characters - long_smiles = "C" * 100 - encoded = encode_smiles(long_smiles, max_length=50) - assert len(encoded) <= 50 - assert "_" in encoded # Hash separator - - def test_max_length_parameter(self): - """Test that max_length parameter controls output length.""" - long_smiles = "C=C" * 20 # 60 chars when encoded - encoded = encode_smiles(long_smiles, max_length=30) - assert len(encoded) <= 30 - - def test_deterministic_encoding(self): - """Test that same input always produces same output.""" - smiles = "CC(=O)OC1=CC=CC=C1C(=O)O" # Aspirin - encoded1 = encode_smiles(smiles) - encoded2 = encode_smiles(smiles) - assert encoded1 == encoded2 - - def test_different_smiles_different_encodings(self): - """Test that different SMILES produce different encodings.""" - encoded1 = encode_smiles("CCO") - encoded2 = encode_smiles("OCC") - assert encoded1 != encoded2 - - -class TestDecodeSmiles: - """Tests for decode_smiles function.""" - - def test_simple_decode(self): - """Test decoding simple SMILES.""" - assert decode_smiles("CCO") == "CCO" - - def test_decode_double_bond(self): - """Test decoding double bonds.""" - assert decode_smiles("CdC") == "C=C" - - def test_decode_triple_bond(self): - """Test decoding triple bonds.""" - assert decode_smiles("CtN") == "C#N" - - def test_decode_charged(self): - """Test decoding charged species.""" - assert decode_smiles("KNH4pJ") == "[NH4+]" - assert decode_smiles("KOmJ") == "[O-]" - - def test_decode_parentheses(self): - """Test decoding parentheses.""" - assert decode_smiles("CCLCRC") == "CC(C)C" - - def test_roundtrip_simple(self): - """Test encode/decode roundtrip for simple SMILES.""" - original = "CCO" - assert decode_smiles(encode_smiles(original)) == original - - def test_roundtrip_complex(self): - """Test encode/decode roundtrip for complex SMILES.""" - original = "C=C(C)C" - assert decode_smiles(encode_smiles(original)) == original - - def test_roundtrip_charged(self): - """Test encode/decode roundtrip for charged species.""" - original = "[NH4+]" - assert decode_smiles(encode_smiles(original)) == original - - def test_hash_encoded_not_decoded(self): - """Test that hash-encoded strings are returned unchanged.""" - # Simulate a hash-encoded string - hash_encoded = "CCCCC_abc123def456" - result = decode_smiles(hash_encoded) - # Should not try to decode the hash portion - assert "_" in result - - class TestHashEnumeratedSmiIDs: """Tests for hash_enumerated_smi_IDs function.""" @@ -414,24 +281,6 @@ def test_incremental_taut_suffix(self, tmp_path): assert all("@taut" in id for id in ids) -class TestHousekeepingHelper: - """Tests for housekeeping_helper function.""" - - def test_moves_file_to_folder(self, tmp_path): - """Test that file is moved to the specified folder.""" - folder = tmp_path / "output" - folder.mkdir() - - source_file = tmp_path / "test.txt" - source_file.write_text("test content") - - housekeeping_helper(str(folder), str(source_file)) - - # File should be in folder now - assert (folder / "test.txt").exists() - assert not source_file.exists() - - class TestHousekeeping: """Tests for housekeeping function."""