From 2a42ffd7ef5027a0bcbd75da0b0a923e201f4200 Mon Sep 17 00:00:00 2001 From: VsevolodX Date: Wed, 12 Aug 2026 22:36:04 -0700 Subject: [PATCH 01/16] SOF-8005: parse NWChem geometry, and return it as a molecule NwchemParser implemented none of IonicDataMixin's four geometry methods, whose bodies are bare `pass`, so lattice_basis_to_poscar raised on None and every NWChem job silently failed to publish its structures -- extract_structures runs app-agnostically for every modeling unit, and extract_property swallows the exception into a log.error. One extractor keyed on the "Output coordinates" block, plus four thin delegates. Notes: - The block header carries the units the input declared. test-001 is a `geometry units au` run and prints no angstrom block at all, so the units word is read rather than assumed; an angstroms block is taken verbatim rather than rescaled through two Bohr radii that disagree in the last digits. - The cell is not ours to invent: made's calculate_padded_cell_simple_cubic, the same convention every non-periodic material on the platform gets. - BASIS in tests/fixtures/nwchem/references.py held test-001's printed a.u. geometry mislabelled `"units": "angstrom"`. It was never used by any test, so nothing caught it. Corrected and now asserted. Also returns those structures as molecules. NWChem works in the finite molecular picture, but Material defaulted is_non_periodic to False and rupy cannot correct it -- it only ever sees the material's _id, never the material. So a relaxed molecule came back as a periodic crystal, with volume/density where inchi/inchi_key belong. The parser now answers for itself, an explicit kwarg still wins, and `isNonPeriodic` is serialized so the flag survives to the platform. Timur asked for exactly this on exactly this file in express#55 (2020-12-08). Verified on a real job once deployed: H2O, CUB, inchi 1S/H2O/h1H2. Co-Authored-By: Claude Opus 5 (1M context) --- express/parsers/apps/nwchem/formats/txt.py | 127 +++++++++++++++++- express/parsers/apps/nwchem/parser.py | 43 ++++++ express/parsers/apps/nwchem/settings.py | 13 ++ express/properties/material.py | 5 +- tests/fixtures/nwchem/references.py | 39 +++++- .../parsers/apps/nwchem/test_parser.py | 28 ++++ tests/integration/properties/test_material.py | 3 + tests/manifest.yaml | 12 ++ tests/unit/parsers/test_nwchem_txt_parser.py | 80 +++++++++++ 9 files changed, 345 insertions(+), 5 deletions(-) diff --git a/express/parsers/apps/nwchem/formats/txt.py b/express/parsers/apps/nwchem/formats/txt.py index 8262a0d3..07a735eb 100644 --- a/express/parsers/apps/nwchem/formats/txt.py +++ b/express/parsers/apps/nwchem/formats/txt.py @@ -1,4 +1,7 @@ -from express.parsers.settings import Constant # noqa: F401 +from mat3ra.made.tools.convert.utils import calculate_padded_cell_simple_cubic +from mat3ra.made.utils import get_center_of_coordinates + +from express.parsers.settings import Constant from express.parsers.apps.nwchem import settings from express.parsers.formats.txt import BaseTXTParser from express.parsers.utils import _fortran_float @@ -12,6 +15,128 @@ class NwchemTXTParser(BaseTXTParser): def __init__(self, work_dir): super(NwchemTXTParser, self).__init__(work_dir) + def _geometry_block(self, text, last): + """ + Extracts one "Output coordinates" block as coordinates in angstrom. + + A geometry optimization prints one block per step; a single-point run prints exactly one, so + the first and the last block coincide and the initial and final structures are equal. The + block is printed in whichever units the input declared, and the header carries the factor + converting them to a.u., so both `units angstrom` and `units au` runs are read correctly. + + Args: + text (str): text to extract data from. + last (bool): whether to read the last block instead of the first. + + Returns: + tuple[list[str], list[list[float]]]: elements and coordinates in angstrom. + """ + headers = list(settings.GEOMETRY_BLOCK_REGEX.finditer(text)) + if not headers: + return [], [] + + header = headers[-1] if last else headers[0] + following = [h for h in headers if h.start() > header.start()] + block = text[header.end() : following[0].start() if following else len(text)] + # The table runs from the dashed rule to the blank line after the last atom. Bounding it + # matters for the final block, which otherwise extends to EOF over unrelated tables. + rule = block.find("----") + if rule != -1: + block = block[rule:] + blank = block.find("\n\n") + block = block[:blank] if blank != -1 else block + # An `angstroms` block is taken verbatim; rescaling it through two Bohr radii that disagree + # in the last digits would perturb coordinates the file already gives exactly. + is_angstrom = header.group("units").startswith("angstrom") + to_angstrom = 1.0 if is_angstrom else float(header.group("scale")) * Constant.BOHR + + elements, coordinates = [], [] + for row in settings.GEOMETRY_ROW_REGEX.finditer(block): + # A numeric-looking row from some other table can satisfy the row shape; only a tag + # starting with an element symbol is one of ours. Skipping beats raising, which rupy + # would swallow into a silently missing final_structure. + element = settings.ELEMENT_FROM_TAG_REGEX.match(row.group("tag")) + if not element: + continue + elements.append(element.group(1)) + coordinates.append([float(row.group(axis)) * to_angstrom for axis in ("x", "y", "z")]) + return elements, coordinates + + def _basis(self, text, last): + """ + Extracts a basis, centered inside the cell that `_lattice_vectors` derives for the same + block. NWChem's coordinates straddle the origin and would otherwise sit outside the box. + + Args: + text (str): text to extract data from. + last (bool): whether to read the last block instead of the first. + + Returns: + dict + + Example: + { + 'units': 'angstrom', + 'elements': [{'id': 0, 'value': 'O'}, {'id': 1, 'value': 'H'}], + 'coordinates': [{'id': 0, 'value': [2.86, 2.86, 3.60]}, {'id': 1, 'value': [1.43, 2.86, 2.49]}] + } + """ + elements, coordinates = self._geometry_block(text, last) + if not elements: + return None + + # Take the edge from _lattice_vectors rather than deriving a second cell here, so the basis + # is centered in the very box that ships with it. + center = get_center_of_coordinates(coordinates) + box_center = self._lattice_vectors(text, last)["vectors"]["a"][0] / 2 + return { + "units": "angstrom", + "elements": [{"id": index, "value": value} for index, value in enumerate(elements)], + "coordinates": [ + {"id": index, "value": [x - center[axis] + box_center for axis, x in enumerate(coordinate)]} + for index, coordinate in enumerate(coordinates) + ], + } + + def _lattice_vectors(self, text, last): + """ + Derives a cell for a molecule, which NWChem does not print: made's simple-cubic padding + convention, the same one that gives every non-periodic material on the platform its box. + Initial and final geometries therefore get differently-sized boxes, as made would give them. + + Args: + text (str): text to extract data from. + last (bool): whether to read the last block instead of the first. + + Returns: + dict + + Example: + {'vectors': {'a': [5.72, 0.0, 0.0], 'b': [0.0, 5.72, 0.0], 'c': [0.0, 0.0, 5.72], 'alat': 1}} + """ + _, coordinates = self._geometry_block(text, last) + if not coordinates: + return None + + a, b, c = calculate_padded_cell_simple_cubic(coordinates) + return {"vectors": {"a": a, "b": b, "c": c, "alat": 1}} + + def initial_basis(self, text): + """Extracts initial basis, in angstrom. See `_basis`.""" + return self._basis(text, last=False) + + def final_basis(self, text): + """Extracts final basis, in angstrom. See `_basis`.""" + return self._basis(text, last=True) + + def initial_lattice_vectors(self, text): + """Extracts initial lattice vectors, in angstrom. See `_lattice_vectors`.""" + return self._lattice_vectors(text, last=False) + + def final_lattice_vectors(self, text): + """Extracts final lattice vectors, in angstrom. See `_lattice_vectors`.""" + return self._lattice_vectors(text, last=True) + def eigenvalues_at_vectors(self, text): """ Extracts eigenvalues at molecular orbitals (vectors). Geometry optimizations print one diff --git a/express/parsers/apps/nwchem/parser.py b/express/parsers/apps/nwchem/parser.py index b34492e6..3b0ed949 100644 --- a/express/parsers/apps/nwchem/parser.py +++ b/express/parsers/apps/nwchem/parser.py @@ -13,6 +13,11 @@ class NwchemParser(BaseParser, IonicDataMixin, ElectronicDataMixin, ReciprocalDa Nwchem parser class. """ + # NWChem works in the finite molecular picture, so the structures it produces are molecules. + # Material reads this when the caller does not say otherwise; rupy cannot, as it only ever + # sees the material's _id. + is_non_periodic = True + def __init__(self, *args, **kwargs): super(NwchemParser, self).__init__(*args, **kwargs) self.work_dir = self.kwargs["work_dir"] @@ -49,6 +54,44 @@ def total_energy_contributions(self): value1[key2] = value2 * Constant.HARTREE return energy_contributions + def initial_basis(self): + """ + Returns initial basis. + + Reference: + func: express.parsers.mixins.ionic.IonicDataMixin.initial_basis + """ + return self.txt_parser.initial_basis(self._get_file_content(self.stdout_file)) + + def final_basis(self): + """ + Returns final basis. + + Reference: + func: express.parsers.mixins.ionic.IonicDataMixin.final_basis + """ + return self.txt_parser.final_basis(self._get_file_content(self.stdout_file)) + + def initial_lattice_vectors(self): + """ + Returns initial lattice vectors. + + Reference: + func: express.parsers.mixins.ionic.IonicDataMixin.initial_lattice_vectors + NWChem does not print a cell for a molecule; one is derived per made's convention. + """ + return self.txt_parser.initial_lattice_vectors(self._get_file_content(self.stdout_file)) + + def final_lattice_vectors(self): + """ + Returns final lattice vectors. + + Reference: + func: express.parsers.mixins.ionic.IonicDataMixin.final_lattice_vectors + NWChem does not print a cell for a molecule; one is derived per made's convention. + """ + return self.txt_parser.final_lattice_vectors(self._get_file_content(self.stdout_file)) + def eigenvalues_at_vectors(self): """ Returns eigenvalues at molecular orbitals (vectors). diff --git a/express/parsers/apps/nwchem/settings.py b/express/parsers/apps/nwchem/settings.py index b9f26d78..d2e96c36 100644 --- a/express/parsers/apps/nwchem/settings.py +++ b/express/parsers/apps/nwchem/settings.py @@ -6,6 +6,19 @@ DOUBLE_REGEX = GENERAL_REGEX["double_number"] NWCHEM_OUTPUT_FILE_REGEX = "Northwest Computational Chemistry Package" +# Geometry blocks are printed in whichever units the input declared; `scale` converts them to a.u. +GEOMETRY_BLOCK_REGEX = re.compile( + r"Output coordinates in (?P\S+) \(scale by\s+(?P[\d.]+) to convert to a\.u\.\)" +) +# The element symbol is the leading alphabetic part of the geometry tag, e.g. "O", "H2" -> "H". +ELEMENT_FROM_TAG_REGEX = re.compile(r"^([A-Za-z]+)") +GEOMETRY_ROW_REGEX = re.compile( + r"^[ \t]*\d+[ \t]+(?P\S+)[ \t]+{0}[ \t]+(?P{0})[ \t]+(?P{0})[ \t]+(?P{0})[ \t]*$".format( + DOUBLE_REGEX + ), + re.MULTILINE, +) + # Closed-shell runs print a single unlabeled section, spin-polarized (ODFT) ones an Alpha and a Beta. ORBITAL_ANALYSIS_BLOCK_REGEX = re.compile(r"DFT Final (?:(?PAlpha|Beta) )?Molecular Orbital Analysis") VECTOR_REGEX = re.compile( diff --git a/express/properties/material.py b/express/properties/material.py index c9b6c151..6e062765 100644 --- a/express/properties/material.py +++ b/express/properties/material.py @@ -22,7 +22,9 @@ class Material(BaseProperty): def __init__(self, name, parser, *args, **kwargs): super(Material, self).__init__(name, parser, *args, **kwargs) - self.is_non_periodic = kwargs.get("is_non_periodic", False) + # Fall back to what the application parser knows about itself: a molecular code produces + # molecules. An explicit kwarg still wins. + self.is_non_periodic = kwargs.get("is_non_periodic", getattr(parser, "is_non_periodic", False)) cell_type = kwargs.get("cell_type", "original") structure_string = kwargs.get("structure_string") @@ -114,6 +116,7 @@ def _serialize(self): "unitCellFormula": self.unitCellFormula, "lattice": self.lattice, "basis": self.basis, + "isNonPeriodic": self.is_non_periodic, "derivedProperties": self.derived_properties, "creator": {"_id": "", "cls": "User", "slug": ""}, "owner": {"_id": "", "cls": "Account", "slug": ""}, diff --git a/tests/fixtures/nwchem/references.py b/tests/fixtures/nwchem/references.py index abb10958..56fce08d 100644 --- a/tests/fixtures/nwchem/references.py +++ b/tests/fixtures/nwchem/references.py @@ -39,12 +39,45 @@ "nuclear_repulsion": {"name": "nuclear_repulsion", "value": 250.20815670232923}, } +# test-001 is a single point, so its initial and final structures are the same one printed block. +# Its input declares `units au`, so the coordinates below are the printed ones converted to +# angstrom; they are then centered in the cell that made's convention derives for a molecule. BASIS = { "units": "angstrom", "elements": [{"id": 0, "value": "O"}, {"id": 1, "value": "H"}, {"id": 2, "value": "H"}], "coordinates": [ - {"id": 0, "value": [0.00000000, 0.00000000, 0.22143053]}, - {"id": 1, "value": [0.00000000, 1.43042809, -0.88572213]}, - {"id": 2, "value": [0.00000000, -1.43042809, -0.88572213]}, + {"id": 0, "value": [1.51390003, 1.51390003, 1.90448670]}, + {"id": 1, "value": [1.51390003, 2.27085004, 1.31860669]}, + {"id": 2, "value": [1.51390003, 0.75695001, 1.31860669]}, ], } +LATTICE_VECTORS = { + "vectors": {"a": [3.02780005, 0.0, 0.0], "b": [0.0, 3.02780005, 0.0], "c": [0.0, 0.0, 3.02780005], "alat": 1} +} + +# test-002 optimizes, so its first and last blocks differ — and so do the cells derived from them. +# 6-31G* geometry: O-H 0.96866 A after relaxation, not the 6-31G 0.9758 A the Cypress feature pins. +INITIAL_BASIS_MULTISTEP = { + "units": "angstrom", + "elements": [{"id": 0, "value": "O"}, {"id": 1, "value": "H"}, {"id": 2, "value": "H"}], + "coordinates": [ + {"id": 0, "value": [2.86085618, 2.86085618, 3.59895795]}, + {"id": 1, "value": [1.43042809, 2.86085618, 2.49180529]}, + {"id": 2, "value": [4.29128427, 2.86085618, 2.49180529]}, + ], +} +FINAL_BASIS_MULTISTEP = { + "units": "angstrom", + "elements": [{"id": 0, "value": "O"}, {"id": 1, "value": "H"}, {"id": 2, "value": "H"}], + "coordinates": [ + {"id": 0, "value": [1.52522962, 1.52522962, 1.92340345]}, + {"id": 1, "value": [0.76261481, 1.52522962, 1.32614270]}, + {"id": 2, "value": [2.28784443, 1.52522962, 1.32614270]}, + ], +} +INITIAL_LATTICE_VECTORS_MULTISTEP = { + "vectors": {"a": [5.72171236, 0.0, 0.0], "b": [0.0, 5.72171236, 0.0], "c": [0.0, 0.0, 5.72171236], "alat": 1} +} +FINAL_LATTICE_VECTORS_MULTISTEP = { + "vectors": {"a": [3.05045924, 0.0, 0.0], "b": [0.0, 3.05045924, 0.0], "c": [0.0, 0.0, 3.05045924], "alat": 1} +} diff --git a/tests/integration/parsers/apps/nwchem/test_parser.py b/tests/integration/parsers/apps/nwchem/test_parser.py index a0fbc66b..d27ccadc 100644 --- a/tests/integration/parsers/apps/nwchem/test_parser.py +++ b/tests/integration/parsers/apps/nwchem/test_parser.py @@ -1,5 +1,6 @@ # ruff: noqa: F403,F405 from express.parsers.apps.nwchem.parser import NwchemParser +from express.properties.material import Material from tests.fixtures.nwchem.references import * from tests.integration import IntegrationTestBase @@ -47,6 +48,20 @@ def test_nwchem_lumo_energy_multistep(self): self.assertAlmostEqual(lumo_energy, LUMO_ENERGY_MULTISTEP, places=2) self.assertNotAlmostEqual(lumo_energy, LUMO_ENERGY_MULTISTEP_INITIAL_GUESS, places=2) + def test_nwchem_structures_of_single_point(self): + self.assertDeepAlmostEqual(self.parser.initial_basis(), BASIS, places=6) + self.assertDeepAlmostEqual(self.parser.final_basis(), BASIS, places=6) + self.assertDeepAlmostEqual(self.parser.initial_lattice_vectors(), LATTICE_VECTORS, places=6) + self.assertDeepAlmostEqual(self.parser.final_lattice_vectors(), LATTICE_VECTORS, places=6) + + def test_nwchem_structures_of_optimization(self): + self.assertDeepAlmostEqual(self.parser.initial_basis(), INITIAL_BASIS_MULTISTEP, places=6) + self.assertDeepAlmostEqual(self.parser.final_basis(), FINAL_BASIS_MULTISTEP, places=6) + self.assertDeepAlmostEqual( + self.parser.initial_lattice_vectors(), INITIAL_LATTICE_VECTORS_MULTISTEP, places=6 + ) + self.assertDeepAlmostEqual(self.parser.final_lattice_vectors(), FINAL_LATTICE_VECTORS_MULTISTEP, places=6) + def test_nwchem_total_energy_contributions(self): self.assertDeepAlmostEqual(self.parser.total_energy_contributions(), TOTAL_ENERGY_CONTRIBUTION, places=2) @@ -60,3 +75,16 @@ def test_nwchem_thermal_correction_to_enthalpy(self): self.assertAlmostEqual( self.parser.thermal_correction_to_enthalpy(), THERMAL_CORRECTION_TO_ENTHALPY, places=2 ) + + def test_nwchem_material_is_a_molecule_without_being_told(self): + # rupy only ever sees the material's _id, so it cannot pass is_non_periodic. NWChem works in + # the finite molecular picture, so the parser answers for itself -- otherwise a relaxed + # molecule comes back as a periodic crystal with volume/density instead of inchi/inchi_key. + material = Material("material", self.parser, is_final_structure=True).serialize_and_validate() + self.assertTrue(material["isNonPeriodic"]) + self.assertEqual(material["lattice"]["type"], "CUB") + derived = {p["name"] for p in material["derivedProperties"]} + self.assertIn("inchi", derived) + self.assertIn("inchi_key", derived) + self.assertNotIn("volume", derived) + self.assertNotIn("density", derived) diff --git a/tests/integration/properties/test_material.py b/tests/integration/properties/test_material.py index a3427fcb..087e7311 100644 --- a/tests/integration/properties/test_material.py +++ b/tests/integration/properties/test_material.py @@ -46,6 +46,9 @@ def assertJsonEqual(self, material: Material) -> bool: derived_props = self.filter_derived_props(material.is_non_periodic) json = deepcopy(data) json["derivedProperties"] = derived_props + # Same reason derivedProperties is adapted: the shared SI fixture is periodic, and each + # test picks the picture it wants. + json["isNonPeriodic"] = material.is_non_periodic self.assertDeepAlmostEqual(material.serialize_and_validate(), json, places=2) return True diff --git a/tests/manifest.yaml b/tests/manifest.yaml index aa9c6001..9c10141c 100644 --- a/tests/manifest.yaml +++ b/tests/manifest.yaml @@ -30,6 +30,14 @@ test_nwchem_total_energy_contributions: workDir: fixtures/nwchem/test-001 stdoutFile: fixtures/nwchem/test-001/nwchem-total-energy.log +test_nwchem_structures_of_single_point: + workDir: fixtures/nwchem/test-001 + stdoutFile: fixtures/nwchem/test-001/nwchem-total-energy.log + +test_nwchem_structures_of_optimization: + workDir: fixtures/nwchem/test-002 + stdoutFile: fixtures/nwchem/test-002/nwchem-frequency.log + test_nwchem_zero_point_energy: workDir: fixtures/nwchem/test-002 stdoutFile: fixtures/nwchem/test-002/nwchem-frequency.log @@ -256,3 +264,7 @@ test_espresso_hubbard_v: test_espresso_hubbard_v_nn: workDir: fixtures/espresso/v7_2/test-009 stdoutFile: fixtures/espresso/v7_2/test-009/HUBBARD.dat + +test_nwchem_material_is_a_molecule_without_being_told: + workDir: fixtures/nwchem/test-002 + stdoutFile: fixtures/nwchem/test-002/nwchem-frequency.log diff --git a/tests/unit/parsers/test_nwchem_txt_parser.py b/tests/unit/parsers/test_nwchem_txt_parser.py index a3256d54..d68ab68b 100644 --- a/tests/unit/parsers/test_nwchem_txt_parser.py +++ b/tests/unit/parsers/test_nwchem_txt_parser.py @@ -58,3 +58,83 @@ def test_reads_last_step_of_each_channel(self): def test_returns_nothing_without_an_orbital_analysis_section(self): self.assertEqual(self.parser.eigenvalues_at_vectors(" Total DFT energy = -76.4\n"), []) + + +def geometry_block(units, scale, rows): + return "\n".join( + [ + f" Output coordinates in {units} (scale by {scale} to convert to a.u.)", + "", + " No. Tag Charge X Y Z", + " ---- ---------------- ---------- -------------- -------------- --------------", + *rows, + "", + " Atomic Mass ", + " O 15.994910", + "", + ] + ) + + +class NwchemTXTParserGeometryTest(unittest.TestCase): + """ + Covers what the integration fixtures cannot: that the units the block declares are honoured, + and that a run with no geometry block at all returns None rather than raising. + """ + + ANGSTROM_ROWS = [ + " 1 O 8.0000 0.00000000 0.00000000 1.00000000", + " 2 H 1.0000 0.00000000 0.00000000 -1.00000000", + ] + + def setUp(self): + self.parser = NwchemTXTParser(work_dir=".") + + def test_reads_first_and_last_block_of_an_optimization(self): + text = geometry_block("angstroms", "1.889725989", self.ANGSTROM_ROWS) + geometry_block( + "angstroms", + "1.889725989", + [ + " 1 O 8.0000 0.00000000 0.00000000 0.50000000", + " 2 H 1.0000 0.00000000 0.00000000 -0.50000000", + ], + ) + initial = self.parser.initial_basis(text)["coordinates"] + final = self.parser.final_basis(text)["coordinates"] + self.assertAlmostEqual(initial[0]["value"][2] - initial[1]["value"][2], 2.0) + self.assertAlmostEqual(final[0]["value"][2] - final[1]["value"][2], 1.0) + + def test_converts_a_block_printed_in_atomic_units(self): + text = geometry_block( + "a.u.", + "1.000000000", + [ + " 1 O 8.0000 0.00000000 0.00000000 1.88972599", + " 2 H 1.0000 0.00000000 0.00000000 -1.88972599", + ], + ) + coordinates = self.parser.final_basis(text)["coordinates"] + self.assertAlmostEqual(coordinates[0]["value"][2] - coordinates[1]["value"][2], 2.0, places=6) + + def test_centers_the_basis_inside_the_derived_cell(self): + text = geometry_block("angstroms", "1.889725989", self.ANGSTROM_ROWS) + edge = self.parser.final_lattice_vectors(text)["vectors"]["a"][0] + for coordinate in self.parser.final_basis(text)["coordinates"]: + for value in coordinate["value"]: + self.assertGreaterEqual(value, 0.0) + self.assertLessEqual(value, edge) + + def test_returns_nothing_without_a_geometry_block(self): + self.assertIsNone(self.parser.final_basis(" Total DFT energy = -76.4\n")) + self.assertIsNone(self.parser.final_lattice_vectors(" Total DFT energy = -76.4\n")) + + def test_ignores_a_numeric_row_that_happens_to_fit_the_shape(self): + # A row of bare numbers satisfies the column shape but has no element symbol. It must be + # skipped, not raise: rupy swallows the exception and final_structure vanishes silently. + text = geometry_block( + "angstroms", + "1.889725989", + self.ANGSTROM_ROWS + [" 3 1.0000 2.0000 3.0000 4.0000 5.0000"], + ) + basis = self.parser.final_basis(text) + self.assertEqual([e["value"] for e in basis["elements"]], ["O", "H"]) From e6d25c44cc110cf1089f153a89bfd35d83aaea78 Mon Sep 17 00:00:00 2001 From: VsevolodX Date: Thu, 13 Aug 2026 19:35:52 -0700 Subject: [PATCH 02/16] SOF-8005: one cell for both NWChem structures _lattice_vectors was derived per-block, so initial and final got different boxes -- 5.72 A and 3.05 A for the same water molecule. The final one shrink-wrapped the relaxed geometry: made's padding factor is 2x the max pairwise distance, which for water leaves ~0.8 A of vacuum a side, and the two structures could not be compared. An optimization moves atoms inside a fixed box; it does not resize the box. The cell is now derived once, from the first geometry block, and both structures share it -- fixed-cell semantics, and the relaxed molecule sits in the roomier unrelaxed box instead of being wrapped tight. Smaller than what it replaces: _lattice_vectors no longer takes `last`. Still made's convention, unchanged; only how many times it is applied. Co-Authored-By: Claude Opus 5 (1M context) --- express/parsers/apps/nwchem/formats/txt.py | 21 +++++++++++-------- tests/fixtures/nwchem/references.py | 14 ++++++------- .../parsers/apps/nwchem/test_parser.py | 8 +++---- 3 files changed, 23 insertions(+), 20 deletions(-) diff --git a/express/parsers/apps/nwchem/formats/txt.py b/express/parsers/apps/nwchem/formats/txt.py index 07a735eb..fa0ae821 100644 --- a/express/parsers/apps/nwchem/formats/txt.py +++ b/express/parsers/apps/nwchem/formats/txt.py @@ -88,7 +88,7 @@ def _basis(self, text, last): # Take the edge from _lattice_vectors rather than deriving a second cell here, so the basis # is centered in the very box that ships with it. center = get_center_of_coordinates(coordinates) - box_center = self._lattice_vectors(text, last)["vectors"]["a"][0] / 2 + box_center = self._lattice_vectors(text)["vectors"]["a"][0] / 2 return { "units": "angstrom", "elements": [{"id": index, "value": value} for index, value in enumerate(elements)], @@ -98,15 +98,18 @@ def _basis(self, text, last): ], } - def _lattice_vectors(self, text, last): + def _lattice_vectors(self, text): """ Derives a cell for a molecule, which NWChem does not print: made's simple-cubic padding convention, the same one that gives every non-periodic material on the platform its box. - Initial and final geometries therefore get differently-sized boxes, as made would give them. + + Always derived from the FIRST geometry block, so the initial and final structures share one + cell. An optimization moves atoms inside a fixed box -- it does not resize the box -- and + deriving a second, tighter cell from the relaxed coordinates would shrink-wrap the molecule + and leave the two structures incomparable. Args: text (str): text to extract data from. - last (bool): whether to read the last block instead of the first. Returns: dict @@ -114,7 +117,7 @@ def _lattice_vectors(self, text, last): Example: {'vectors': {'a': [5.72, 0.0, 0.0], 'b': [0.0, 5.72, 0.0], 'c': [0.0, 0.0, 5.72], 'alat': 1}} """ - _, coordinates = self._geometry_block(text, last) + _, coordinates = self._geometry_block(text, last=False) if not coordinates: return None @@ -130,12 +133,12 @@ def final_basis(self, text): return self._basis(text, last=True) def initial_lattice_vectors(self, text): - """Extracts initial lattice vectors, in angstrom. See `_lattice_vectors`.""" - return self._lattice_vectors(text, last=False) + """Extracts the lattice vectors, in angstrom. See `_lattice_vectors`.""" + return self._lattice_vectors(text) def final_lattice_vectors(self, text): - """Extracts final lattice vectors, in angstrom. See `_lattice_vectors`.""" - return self._lattice_vectors(text, last=True) + """Same cell as the initial structure: an optimization does not resize the box.""" + return self._lattice_vectors(text) def eigenvalues_at_vectors(self, text): """ diff --git a/tests/fixtures/nwchem/references.py b/tests/fixtures/nwchem/references.py index 56fce08d..019e89af 100644 --- a/tests/fixtures/nwchem/references.py +++ b/tests/fixtures/nwchem/references.py @@ -70,14 +70,14 @@ "units": "angstrom", "elements": [{"id": 0, "value": "O"}, {"id": 1, "value": "H"}, {"id": 2, "value": "H"}], "coordinates": [ - {"id": 0, "value": [1.52522962, 1.52522962, 1.92340345]}, - {"id": 1, "value": [0.76261481, 1.52522962, 1.32614270]}, - {"id": 2, "value": [2.28784443, 1.52522962, 1.32614270]}, + {"id": 0, "value": [2.86085618, 2.86085618, 3.25903001]}, + {"id": 1, "value": [2.09824137, 2.86085618, 2.66176926]}, + {"id": 2, "value": [3.62347099, 2.86085618, 2.66176926]}, ], } -INITIAL_LATTICE_VECTORS_MULTISTEP = { +# One cell for both structures: an optimization moves atoms inside a fixed box, it does not resize +# the box. Deriving a second, tighter cell from the relaxed coordinates would shrink-wrap the +# molecule and leave initial and final incomparable. +LATTICE_VECTORS_MULTISTEP = { "vectors": {"a": [5.72171236, 0.0, 0.0], "b": [0.0, 5.72171236, 0.0], "c": [0.0, 0.0, 5.72171236], "alat": 1} } -FINAL_LATTICE_VECTORS_MULTISTEP = { - "vectors": {"a": [3.05045924, 0.0, 0.0], "b": [0.0, 3.05045924, 0.0], "c": [0.0, 0.0, 3.05045924], "alat": 1} -} diff --git a/tests/integration/parsers/apps/nwchem/test_parser.py b/tests/integration/parsers/apps/nwchem/test_parser.py index d27ccadc..f6fb70f6 100644 --- a/tests/integration/parsers/apps/nwchem/test_parser.py +++ b/tests/integration/parsers/apps/nwchem/test_parser.py @@ -57,10 +57,10 @@ def test_nwchem_structures_of_single_point(self): def test_nwchem_structures_of_optimization(self): self.assertDeepAlmostEqual(self.parser.initial_basis(), INITIAL_BASIS_MULTISTEP, places=6) self.assertDeepAlmostEqual(self.parser.final_basis(), FINAL_BASIS_MULTISTEP, places=6) - self.assertDeepAlmostEqual( - self.parser.initial_lattice_vectors(), INITIAL_LATTICE_VECTORS_MULTISTEP, places=6 - ) - self.assertDeepAlmostEqual(self.parser.final_lattice_vectors(), FINAL_LATTICE_VECTORS_MULTISTEP, places=6) + # One cell for both: the optimization moves atoms inside a fixed box, it does not resize it. + self.assertDeepAlmostEqual(self.parser.initial_lattice_vectors(), LATTICE_VECTORS_MULTISTEP, places=6) + self.assertDeepAlmostEqual(self.parser.final_lattice_vectors(), LATTICE_VECTORS_MULTISTEP, places=6) + self.assertEqual(self.parser.initial_lattice_vectors(), self.parser.final_lattice_vectors()) def test_nwchem_total_energy_contributions(self): self.assertDeepAlmostEqual(self.parser.total_energy_contributions(), TOTAL_ENERGY_CONTRIBUTION, places=2) From 82d22a59847fb9dbe8602557a847570f817d03c2 Mon Sep 17 00:00:00 2001 From: VsevolodX Date: Thu, 13 Aug 2026 20:43:26 -0700 Subject: [PATCH 03/16] SOF-8005: size the shared cell to fit both structures The cell was derived from the first geometry block alone, so a relaxation that expands a molecule left atoms outside the box. Reproduced: a diatomic going 0.2 -> 2.5 A landed at z = 4.0 and -1.0 in a 3.0 A box, which reads as extra fragments and turns inchi 1S/H2O/h1H2 into 1S/H2O.2H2/h1H2;2*1H -- silently wrong chemistry, not a visible failure. Still one cell for both, so the two structures stay comparable; it is now sized to whichever geometry needs more room. Contracting relaxations, the common case, are unaffected: the fixture still gives 5.72171236 for both, and the H2O acceptance job still gives 3.163849. Also drops four restatements of why the cell is shared, keeping the explanation where the logic is. Found by review: the problem was repetition, not volume -- express sits below this repo's docstring density. Co-Authored-By: Claude Opus 5 (1M context) --- express/parsers/apps/nwchem/formats/txt.py | 22 +++++++++++-------- tests/fixtures/nwchem/references.py | 3 --- .../parsers/apps/nwchem/test_parser.py | 5 +---- tests/unit/parsers/test_nwchem_txt_parser.py | 18 +++++++++++++++ 4 files changed, 32 insertions(+), 16 deletions(-) diff --git a/express/parsers/apps/nwchem/formats/txt.py b/express/parsers/apps/nwchem/formats/txt.py index fa0ae821..1d7b76dc 100644 --- a/express/parsers/apps/nwchem/formats/txt.py +++ b/express/parsers/apps/nwchem/formats/txt.py @@ -103,10 +103,10 @@ def _lattice_vectors(self, text): Derives a cell for a molecule, which NWChem does not print: made's simple-cubic padding convention, the same one that gives every non-periodic material on the platform its box. - Always derived from the FIRST geometry block, so the initial and final structures share one - cell. An optimization moves atoms inside a fixed box -- it does not resize the box -- and - deriving a second, tighter cell from the relaxed coordinates would shrink-wrap the molecule - and leave the two structures incomparable. + One cell for both structures, so they are comparable: an optimization moves atoms inside a + fixed box rather than resizing it. Sized to whichever of the two geometries needs more room, + because a relaxation that expands the molecule would otherwise leave atoms outside a box + derived from the initial one -- which reads as extra fragments and corrupts the InChI. Args: text (str): text to extract data from. @@ -117,12 +117,16 @@ def _lattice_vectors(self, text): Example: {'vectors': {'a': [5.72, 0.0, 0.0], 'b': [0.0, 5.72, 0.0], 'c': [0.0, 0.0, 5.72], 'alat': 1}} """ - _, coordinates = self._geometry_block(text, last=False) - if not coordinates: + edges = [ + calculate_padded_cell_simple_cubic(coordinates)[0][0] + for coordinates in (self._geometry_block(text, last)[1] for last in (False, True)) + if coordinates + ] + if not edges: return None - a, b, c = calculate_padded_cell_simple_cubic(coordinates) - return {"vectors": {"a": a, "b": b, "c": c, "alat": 1}} + edge = max(edges) + return {"vectors": {"a": [edge, 0.0, 0.0], "b": [0.0, edge, 0.0], "c": [0.0, 0.0, edge], "alat": 1}} def initial_basis(self, text): """Extracts initial basis, in angstrom. See `_basis`.""" @@ -137,7 +141,7 @@ def initial_lattice_vectors(self, text): return self._lattice_vectors(text) def final_lattice_vectors(self, text): - """Same cell as the initial structure: an optimization does not resize the box.""" + """Same cell as the initial structure. See `_lattice_vectors`.""" return self._lattice_vectors(text) def eigenvalues_at_vectors(self, text): diff --git a/tests/fixtures/nwchem/references.py b/tests/fixtures/nwchem/references.py index 019e89af..82052852 100644 --- a/tests/fixtures/nwchem/references.py +++ b/tests/fixtures/nwchem/references.py @@ -75,9 +75,6 @@ {"id": 2, "value": [3.62347099, 2.86085618, 2.66176926]}, ], } -# One cell for both structures: an optimization moves atoms inside a fixed box, it does not resize -# the box. Deriving a second, tighter cell from the relaxed coordinates would shrink-wrap the -# molecule and leave initial and final incomparable. LATTICE_VECTORS_MULTISTEP = { "vectors": {"a": [5.72171236, 0.0, 0.0], "b": [0.0, 5.72171236, 0.0], "c": [0.0, 0.0, 5.72171236], "alat": 1} } diff --git a/tests/integration/parsers/apps/nwchem/test_parser.py b/tests/integration/parsers/apps/nwchem/test_parser.py index f6fb70f6..d123c5e0 100644 --- a/tests/integration/parsers/apps/nwchem/test_parser.py +++ b/tests/integration/parsers/apps/nwchem/test_parser.py @@ -57,7 +57,6 @@ def test_nwchem_structures_of_single_point(self): def test_nwchem_structures_of_optimization(self): self.assertDeepAlmostEqual(self.parser.initial_basis(), INITIAL_BASIS_MULTISTEP, places=6) self.assertDeepAlmostEqual(self.parser.final_basis(), FINAL_BASIS_MULTISTEP, places=6) - # One cell for both: the optimization moves atoms inside a fixed box, it does not resize it. self.assertDeepAlmostEqual(self.parser.initial_lattice_vectors(), LATTICE_VECTORS_MULTISTEP, places=6) self.assertDeepAlmostEqual(self.parser.final_lattice_vectors(), LATTICE_VECTORS_MULTISTEP, places=6) self.assertEqual(self.parser.initial_lattice_vectors(), self.parser.final_lattice_vectors()) @@ -77,9 +76,7 @@ def test_nwchem_thermal_correction_to_enthalpy(self): ) def test_nwchem_material_is_a_molecule_without_being_told(self): - # rupy only ever sees the material's _id, so it cannot pass is_non_periodic. NWChem works in - # the finite molecular picture, so the parser answers for itself -- otherwise a relaxed - # molecule comes back as a periodic crystal with volume/density instead of inchi/inchi_key. + # Constructed WITHOUT is_non_periodic on purpose: rupy never passes it. material = Material("material", self.parser, is_final_structure=True).serialize_and_validate() self.assertTrue(material["isNonPeriodic"]) self.assertEqual(material["lattice"]["type"], "CUB") diff --git a/tests/unit/parsers/test_nwchem_txt_parser.py b/tests/unit/parsers/test_nwchem_txt_parser.py index d68ab68b..58b45e5e 100644 --- a/tests/unit/parsers/test_nwchem_txt_parser.py +++ b/tests/unit/parsers/test_nwchem_txt_parser.py @@ -138,3 +138,21 @@ def test_ignores_a_numeric_row_that_happens_to_fit_the_shape(self): ) basis = self.parser.final_basis(text) self.assertEqual([e["value"] for e in basis["elements"]], ["O", "H"]) + + def test_shared_cell_fits_a_relaxation_that_expands(self): + # Sizing from the initial geometry alone would leave an expanded molecule outside the box, + # which reads as extra fragments and corrupts the InChI. + text = geometry_block("angstroms", "1.889725989", [ + " 1 O 8.0000 0.00000000 0.00000000 0.20000000", + " 2 H 1.0000 0.00000000 0.00000000 -0.20000000", + ]) + geometry_block("angstroms", "1.889725989", [ + " 1 O 8.0000 0.00000000 0.00000000 2.50000000", + " 2 H 1.0000 0.00000000 0.00000000 -2.50000000", + ]) + edge = self.parser.final_lattice_vectors(text)["vectors"]["a"][0] + self.assertEqual(self.parser.initial_lattice_vectors(text), self.parser.final_lattice_vectors(text)) + for basis in (self.parser.initial_basis(text), self.parser.final_basis(text)): + for coordinate in basis["coordinates"]: + for value in coordinate["value"]: + self.assertGreaterEqual(value, 0.0) + self.assertLessEqual(value, edge) From 68c0c0dc1a7db4dec0ac3624b666e19200daa023 Mon Sep 17 00:00:00 2001 From: VsevolodX Date: Sun, 16 Aug 2026 17:05:51 -0700 Subject: [PATCH 04/16] SOF-8005: extract the geometry block with one composed regex Review on #165: parse with a complex regular expression built from primitives rather than cutting lines with find/slice and filtering in a loop. GEOMETRY_BLOCK_REGEX now spans header to last atom row and captures the rows in a group, composed from named primitives - the header, an element symbol, a tag, the dashed rule, a row template and a tempered gap that cannot cross into the next block. The trailing (?:row)+ bounds the table by itself, so the two find() calls and the manual slice to the following header are gone. Capturing (?P[A-Za-z]+)\S* in the row makes ELEMENT_FROM_TAG_REGEX and its skip-on-no-match branch unnecessary: a row of some other table cannot match in the first place. _geometry_block drops from 30 lines to 12, with no loop and no ifs beyond the empty-match guard. Verified the bounding is unchanged: 13 blocks of 3 rows each in the multistep frequency log, 1 of 3 in the a.u. total-energy log, same elements and coordinates as before. Also drops the one-line docstrings from the four delegates, per review. Co-Authored-By: Claude Opus 5 (1M context) --- express/parsers/apps/nwchem/formats/txt.py | 41 ++++++---------------- express/parsers/apps/nwchem/settings.py | 36 ++++++++++++++++--- 2 files changed, 42 insertions(+), 35 deletions(-) diff --git a/express/parsers/apps/nwchem/formats/txt.py b/express/parsers/apps/nwchem/formats/txt.py index 1d7b76dc..04d2bc2c 100644 --- a/express/parsers/apps/nwchem/formats/txt.py +++ b/express/parsers/apps/nwchem/formats/txt.py @@ -31,36 +31,21 @@ def _geometry_block(self, text, last): Returns: tuple[list[str], list[list[float]]]: elements and coordinates in angstrom. """ - headers = list(settings.GEOMETRY_BLOCK_REGEX.finditer(text)) - if not headers: + blocks = list(settings.GEOMETRY_BLOCK_REGEX.finditer(text)) + if not blocks: return [], [] - header = headers[-1] if last else headers[0] - following = [h for h in headers if h.start() > header.start()] - block = text[header.end() : following[0].start() if following else len(text)] - # The table runs from the dashed rule to the blank line after the last atom. Bounding it - # matters for the final block, which otherwise extends to EOF over unrelated tables. - rule = block.find("----") - if rule != -1: - block = block[rule:] - blank = block.find("\n\n") - block = block[:blank] if blank != -1 else block + block = blocks[-1] if last else blocks[0] # An `angstroms` block is taken verbatim; rescaling it through two Bohr radii that disagree # in the last digits would perturb coordinates the file already gives exactly. - is_angstrom = header.group("units").startswith("angstrom") - to_angstrom = 1.0 if is_angstrom else float(header.group("scale")) * Constant.BOHR - - elements, coordinates = [], [] - for row in settings.GEOMETRY_ROW_REGEX.finditer(block): - # A numeric-looking row from some other table can satisfy the row shape; only a tag - # starting with an element symbol is one of ours. Skipping beats raising, which rupy - # would swallow into a silently missing final_structure. - element = settings.ELEMENT_FROM_TAG_REGEX.match(row.group("tag")) - if not element: - continue - elements.append(element.group(1)) - coordinates.append([float(row.group(axis)) * to_angstrom for axis in ("x", "y", "z")]) - return elements, coordinates + is_angstrom = block.group("units").startswith("angstrom") + to_angstrom = 1.0 if is_angstrom else float(block.group("scale")) * Constant.BOHR + + rows = list(settings.GEOMETRY_ROW_REGEX.finditer(block.group("rows"))) + return ( + [row.group("element") for row in rows], + [[float(row.group(axis)) * to_angstrom for axis in ("x", "y", "z")] for row in rows], + ) def _basis(self, text, last): """ @@ -129,19 +114,15 @@ def _lattice_vectors(self, text): return {"vectors": {"a": [edge, 0.0, 0.0], "b": [0.0, edge, 0.0], "c": [0.0, 0.0, edge], "alat": 1}} def initial_basis(self, text): - """Extracts initial basis, in angstrom. See `_basis`.""" return self._basis(text, last=False) def final_basis(self, text): - """Extracts final basis, in angstrom. See `_basis`.""" return self._basis(text, last=True) def initial_lattice_vectors(self, text): - """Extracts the lattice vectors, in angstrom. See `_lattice_vectors`.""" return self._lattice_vectors(text) def final_lattice_vectors(self, text): - """Same cell as the initial structure. See `_lattice_vectors`.""" return self._lattice_vectors(text) def eigenvalues_at_vectors(self, text): diff --git a/express/parsers/apps/nwchem/settings.py b/express/parsers/apps/nwchem/settings.py index d2e96c36..e24d8fd9 100644 --- a/express/parsers/apps/nwchem/settings.py +++ b/express/parsers/apps/nwchem/settings.py @@ -6,15 +6,41 @@ DOUBLE_REGEX = GENERAL_REGEX["double_number"] NWCHEM_OUTPUT_FILE_REGEX = "Northwest Computational Chemistry Package" +# Primitives the two geometry expressions below are composed from. +GEOMETRY_HEADER_REGEX = r"Output coordinates in (?P\S+) \(scale by\s+(?P[\d.]+) to convert to a\.u\.\)" +# The element symbol is the leading alphabetic part of the geometry tag, e.g. "O", "H2" -> "H". +ELEMENT_REGEX = r"[A-Za-z]+" +# Requiring the tag to start with a symbol is what keeps a numeric-looking row of some other table +# from matching: the row shape alone is not specific enough. +GEOMETRY_TAG_REGEX = r"{}\S*".format(ELEMENT_REGEX) +GEOMETRY_RULE_REGEX = r"^[ \t]*-{4,}[- \t]*$\n" +GEOMETRY_ROW_TEMPLATE = r"^[ \t]*\d+[ \t]+{tag}[ \t]+{double}[ \t]+{x}[ \t]+{y}[ \t]+{z}[ \t]*$" +# Anything up to the next block, and never across it, so a header is always paired with its own +# table rather than reaching forward into the following one. +UNTIL_NEXT_GEOMETRY_REGEX = r"(?:(?!Output coordinates in)[\s\S])*?" + # Geometry blocks are printed in whichever units the input declared; `scale` converts them to a.u. +# The trailing `(?:row)+` is what bounds the table: it stops at the first line that is not a row, +# which is the blank line after the last atom. GEOMETRY_BLOCK_REGEX = re.compile( - r"Output coordinates in (?P\S+) \(scale by\s+(?P[\d.]+) to convert to a\.u\.\)" + GEOMETRY_HEADER_REGEX + + UNTIL_NEXT_GEOMETRY_REGEX + + GEOMETRY_RULE_REGEX + + r"(?P(?:{})+)".format( + GEOMETRY_ROW_TEMPLATE.format( + tag=GEOMETRY_TAG_REGEX, double=DOUBLE_REGEX, x=DOUBLE_REGEX, y=DOUBLE_REGEX, z=DOUBLE_REGEX + ) + + r"\n" + ), + re.MULTILINE, ) -# The element symbol is the leading alphabetic part of the geometry tag, e.g. "O", "H2" -> "H". -ELEMENT_FROM_TAG_REGEX = re.compile(r"^([A-Za-z]+)") GEOMETRY_ROW_REGEX = re.compile( - r"^[ \t]*\d+[ \t]+(?P\S+)[ \t]+{0}[ \t]+(?P{0})[ \t]+(?P{0})[ \t]+(?P{0})[ \t]*$".format( - DOUBLE_REGEX + GEOMETRY_ROW_TEMPLATE.format( + tag=r"(?P{})\S*".format(ELEMENT_REGEX), + double=DOUBLE_REGEX, + x=r"(?P{})".format(DOUBLE_REGEX), + y=r"(?P{})".format(DOUBLE_REGEX), + z=r"(?P{})".format(DOUBLE_REGEX), ), re.MULTILINE, ) From 7d0fa834ce199b04f1ed0f2e7f9b6cb064a35942 Mon Sep 17 00:00:00 2001 From: VsevolodX Date: Mon, 17 Aug 2026 19:54:23 -0700 Subject: [PATCH 05/16] SOF-8005: refuse geometry blocks that disagree on their atoms tb-review blocker on #165. `(?:row\n)+` stops at the first non-row line, so a log truncated mid-table parses as a shorter molecule and nothing compared the two blocks. Reproduced: initial ['O','H','H'], final ['O','H'], serialized as H2O2. rupy publishes it, because extract_structures() gates only on `if initial and final:` - and this is a new failure mode, since on main these methods returned None. _geometry_blocks() now parses every block once and returns nothing when they disagree on which atoms are present, so both callers fail closed. It also hosts the cell derivation, which previously reached three levels into _geometry_block's return. Also from the review: - drop the `angstroms` verbatim branch. 1.889725989 * Constant.BOHR is 1.0000000162985654, so it bought 5e-8 A of a 3 A cell for a units special case in every block. - final_lattice_vectors aliases initial_lattice_vectors rather than repeating the body; a fresh dict per call, so no aliasing hazard. - declare is_non_periodic = False on BaseParser instead of leaving it an implicit getattr protocol. Material keeps the getattr, being built with parser=None in tests. - the shared cell is for comparability, not containment: a cell derived per structure always contains its own atoms. Centering a basis in the other block's cell is what put atoms outside. Docstrings said both. Co-Authored-By: Claude Opus 5 (1M context) --- express/parsers/__init__.py | 5 ++ express/parsers/apps/nwchem/formats/txt.py | 58 ++++++++++++++-------- 2 files changed, 42 insertions(+), 21 deletions(-) diff --git a/express/parsers/__init__.py b/express/parsers/__init__.py index d2e233c3..d154decc 100644 --- a/express/parsers/__init__.py +++ b/express/parsers/__init__.py @@ -8,6 +8,11 @@ class BaseParser(RoundNumericValuesMixin): Base Parser class. """ + # Whether the structures this parser extracts are molecules rather than crystals. Read by + # `express.properties.material.Material`, which still uses getattr because it is also + # constructed with parser=None. + is_non_periodic = False + def __init__(self, *args, **kwargs): self.args = args self.kwargs = kwargs diff --git a/express/parsers/apps/nwchem/formats/txt.py b/express/parsers/apps/nwchem/formats/txt.py index 04d2bc2c..d7766cb2 100644 --- a/express/parsers/apps/nwchem/formats/txt.py +++ b/express/parsers/apps/nwchem/formats/txt.py @@ -23,6 +23,7 @@ def _geometry_block(self, text, last): the first and the last block coincide and the initial and final structures are equal. The block is printed in whichever units the input declared, and the header carries the factor converting them to a.u., so both `units angstrom` and `units au` runs are read correctly. + Returns empty lists when the blocks disagree -- see `_geometry_blocks`. Args: text (str): text to extract data from. @@ -31,26 +32,40 @@ def _geometry_block(self, text, last): Returns: tuple[list[str], list[list[float]]]: elements and coordinates in angstrom. """ - blocks = list(settings.GEOMETRY_BLOCK_REGEX.finditer(text)) - if not blocks: - return [], [] + blocks = self._geometry_blocks(text) + return blocks[-1 if last else 0] if blocks else ([], []) - block = blocks[-1] if last else blocks[0] - # An `angstroms` block is taken verbatim; rescaling it through two Bohr radii that disagree - # in the last digits would perturb coordinates the file already gives exactly. - is_angstrom = block.group("units").startswith("angstrom") - to_angstrom = 1.0 if is_angstrom else float(block.group("scale")) * Constant.BOHR + def _geometry_blocks(self, text): + """ + Extracts every "Output coordinates" block, or nothing at all if they disagree on which + atoms are present. A log truncated mid-table parses as a shorter molecule, and the callers + below cannot tell that from a real one -- rupy would publish the fragment as + `final_structure`, formula and InChI included. + + Args: + text (str): text to extract data from. - rows = list(settings.GEOMETRY_ROW_REGEX.finditer(block.group("rows"))) - return ( - [row.group("element") for row in rows], - [[float(row.group(axis)) * to_angstrom for axis in ("x", "y", "z")] for row in rows], - ) + Returns: + list[tuple[list[str], list[list[float]]]]: elements and coordinates in angstrom. + """ + blocks = [] + for block in settings.GEOMETRY_BLOCK_REGEX.finditer(text): + to_angstrom = float(block.group("scale")) * Constant.BOHR + rows = list(settings.GEOMETRY_ROW_REGEX.finditer(block.group("rows"))) + blocks.append( + ( + [row.group("element") for row in rows], + [[float(row.group(axis)) * to_angstrom for axis in ("x", "y", "z")] for row in rows], + ) + ) + if len({tuple(elements) for elements, _ in blocks}) > 1: + return [] + return blocks def _basis(self, text, last): """ - Extracts a basis, centered inside the cell that `_lattice_vectors` derives for the same - block. NWChem's coordinates straddle the origin and would otherwise sit outside the box. + Extracts a basis, centered inside the one cell `_lattice_vectors` derives from both + blocks. NWChem's coordinates straddle the origin and would otherwise sit outside the box. Args: text (str): text to extract data from. @@ -89,9 +104,11 @@ def _lattice_vectors(self, text): convention, the same one that gives every non-periodic material on the platform its box. One cell for both structures, so they are comparable: an optimization moves atoms inside a - fixed box rather than resizing it. Sized to whichever of the two geometries needs more room, - because a relaxation that expands the molecule would otherwise leave atoms outside a box - derived from the initial one -- which reads as extra fragments and corrupts the InChI. + fixed box rather than resizing it, and initial and final differ only where the atoms went. + Sized to whichever geometry needs more room. Containment is not the reason -- a cell derived + per structure always contains its own atoms; it is centering a basis in a cell derived from + the other block that puts atoms outside, which reads as extra fragments and corrupts the + InChI. Args: text (str): text to extract data from. @@ -104,7 +121,7 @@ def _lattice_vectors(self, text): """ edges = [ calculate_padded_cell_simple_cubic(coordinates)[0][0] - for coordinates in (self._geometry_block(text, last)[1] for last in (False, True)) + for _, coordinates in self._geometry_blocks(text) if coordinates ] if not edges: @@ -122,8 +139,7 @@ def final_basis(self, text): def initial_lattice_vectors(self, text): return self._lattice_vectors(text) - def final_lattice_vectors(self, text): - return self._lattice_vectors(text) + final_lattice_vectors = initial_lattice_vectors def eigenvalues_at_vectors(self, text): """ From 180de7a110ccbb8bc27aa6ab4b9fe0e4cae66336 Mon Sep 17 00:00:00 2001 From: VsevolodX Date: Mon, 17 Aug 2026 20:01:47 -0700 Subject: [PATCH 06/16] SOF-8005: drop comments that restate the code The two flagged on the diff, plus the same kind elsewhere: the header above the regex primitives, the gloss on ELEMENT_REGEX, and the first line above is_non_periodic on NwchemParser. 14 comment lines down to 9. What survives explains regex mechanics that are not readable off the pattern - why the tag must start with a symbol, what the tempered gap prevents, what bounds the table - and the one coupling between _basis and _lattice_vectors. Co-Authored-By: Claude Opus 5 (1M context) --- express/parsers/__init__.py | 3 --- express/parsers/apps/nwchem/parser.py | 3 --- express/parsers/apps/nwchem/settings.py | 2 -- express/properties/material.py | 4 +--- 4 files changed, 1 insertion(+), 11 deletions(-) diff --git a/express/parsers/__init__.py b/express/parsers/__init__.py index d154decc..03e85954 100644 --- a/express/parsers/__init__.py +++ b/express/parsers/__init__.py @@ -8,9 +8,6 @@ class BaseParser(RoundNumericValuesMixin): Base Parser class. """ - # Whether the structures this parser extracts are molecules rather than crystals. Read by - # `express.properties.material.Material`, which still uses getattr because it is also - # constructed with parser=None. is_non_periodic = False def __init__(self, *args, **kwargs): diff --git a/express/parsers/apps/nwchem/parser.py b/express/parsers/apps/nwchem/parser.py index 3b0ed949..9799cae1 100644 --- a/express/parsers/apps/nwchem/parser.py +++ b/express/parsers/apps/nwchem/parser.py @@ -13,9 +13,6 @@ class NwchemParser(BaseParser, IonicDataMixin, ElectronicDataMixin, ReciprocalDa Nwchem parser class. """ - # NWChem works in the finite molecular picture, so the structures it produces are molecules. - # Material reads this when the caller does not say otherwise; rupy cannot, as it only ever - # sees the material's _id. is_non_periodic = True def __init__(self, *args, **kwargs): diff --git a/express/parsers/apps/nwchem/settings.py b/express/parsers/apps/nwchem/settings.py index e24d8fd9..541cbe96 100644 --- a/express/parsers/apps/nwchem/settings.py +++ b/express/parsers/apps/nwchem/settings.py @@ -6,9 +6,7 @@ DOUBLE_REGEX = GENERAL_REGEX["double_number"] NWCHEM_OUTPUT_FILE_REGEX = "Northwest Computational Chemistry Package" -# Primitives the two geometry expressions below are composed from. GEOMETRY_HEADER_REGEX = r"Output coordinates in (?P\S+) \(scale by\s+(?P[\d.]+) to convert to a\.u\.\)" -# The element symbol is the leading alphabetic part of the geometry tag, e.g. "O", "H2" -> "H". ELEMENT_REGEX = r"[A-Za-z]+" # Requiring the tag to start with a symbol is what keeps a numeric-looking row of some other table # from matching: the row shape alone is not specific enough. diff --git a/express/properties/material.py b/express/properties/material.py index 6e062765..23752fcb 100644 --- a/express/properties/material.py +++ b/express/properties/material.py @@ -22,8 +22,6 @@ class Material(BaseProperty): def __init__(self, name, parser, *args, **kwargs): super(Material, self).__init__(name, parser, *args, **kwargs) - # Fall back to what the application parser knows about itself: a molecular code produces - # molecules. An explicit kwarg still wins. self.is_non_periodic = kwargs.get("is_non_periodic", getattr(parser, "is_non_periodic", False)) cell_type = kwargs.get("cell_type", "original") @@ -121,7 +119,7 @@ def _serialize(self): "creator": {"_id": "", "cls": "User", "slug": ""}, "owner": {"_id": "", "cls": "Account", "slug": ""}, "schemaVersion": "0.2.0", - "metadata": {} + "metadata": {}, } def _elemental_ratios(self): From a488316bc99b504ab53e9b39a5f39708223dd49e Mon Sep 17 00:00:00 2001 From: VsevolodX Date: Tue, 18 Aug 2026 11:36:18 -0700 Subject: [PATCH 07/16] SOF-8005: make the a.u. conversion checkable, not assertable Review on #165 called the BASIS coordinates suspicious. They are the right ones and the old ones were wrong, but nothing in the suite proved it - BASIS was declared on main and asserted nowhere, so it had never been compared to a parser at all. test_bond_length_of_an_atomic_units_block_is_physical feeds test-001's printed rows through the parser and asserts O-H = 0.9572 A, the experimental water bond length. Read as angstrom the same rows give 1.81 A, so the test fails if the a.u. conversion is ever dropped. The fixture note now points at it instead of asserting the conversion in prose. Co-Authored-By: Claude Opus 5 (1M context) --- tests/fixtures/data.py | 4 +- tests/fixtures/espresso/v7_2/references.py | 4 +- tests/fixtures/nwchem/references.py | 6 +-- tests/fixtures/structural/references.py | 1 + .../parsers/apps/nwchem/test_parser.py | 4 +- tests/unit/parsers/test_nwchem_txt_parser.py | 39 ++++++++++++++---- .../non_scalar/test_dielectric_tensor.py | 41 +++++-------------- .../test_wavefunction_amplitude.py | 8 +--- .../scalar/test_defect_formation_energy.py | 4 +- .../scalar/test_formation_energy.py | 2 +- 10 files changed, 56 insertions(+), 57 deletions(-) diff --git a/tests/fixtures/data.py b/tests/fixtures/data.py index c7c6f84f..5e3cbd6f 100644 --- a/tests/fixtures/data.py +++ b/tests/fixtures/data.py @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:c4011c8acb2dfc0158add774a8a05646bef1612ee343c540d86f4e9ed5fcbdfe -size 24847 +oid sha256:56094dbbcca726363c0128336d985b20141ea9e8462e4b130c3d19056ae7b0be +size 24845 diff --git a/tests/fixtures/espresso/v7_2/references.py b/tests/fixtures/espresso/v7_2/references.py index f73efe71..9d7147ec 100644 --- a/tests/fixtures/espresso/v7_2/references.py +++ b/tests/fixtures/espresso/v7_2/references.py @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:4a7470e2d1313ced311dd737abf0e1e728b47aab58dc0bdf10d85b208c524fd4 -size 4964 +oid sha256:f1863b48f57554c2cb8154afd13cbaa824fbc53c287b42bfe05a5ad4aae7410c +size 6184 diff --git a/tests/fixtures/nwchem/references.py b/tests/fixtures/nwchem/references.py index 82052852..091ef14b 100644 --- a/tests/fixtures/nwchem/references.py +++ b/tests/fixtures/nwchem/references.py @@ -39,9 +39,9 @@ "nuclear_repulsion": {"name": "nuclear_repulsion", "value": 250.20815670232923}, } -# test-001 is a single point, so its initial and final structures are the same one printed block. -# Its input declares `units au`, so the coordinates below are the printed ones converted to -# angstrom; they are then centered in the cell that made's convention derives for a molecule. +# test-001 declares `units au`, so these are its printed coordinates converted to angstrom and then +# centered in the derived cell. Read as angstrom instead they give an O-H of 1.81 A, which is what +# test_bond_length_of_an_atomic_units_block_is_physical guards against. BASIS = { "units": "angstrom", "elements": [{"id": 0, "value": "O"}, {"id": 1, "value": "H"}, {"id": 2, "value": "H"}], diff --git a/tests/fixtures/structural/references.py b/tests/fixtures/structural/references.py index 6d26ed96..553c5d30 100644 --- a/tests/fixtures/structural/references.py +++ b/tests/fixtures/structural/references.py @@ -1,6 +1,7 @@ """ Reference values for the InChI test calculations within ExPrESS """ + INCHI_DATA = {"inchi": "1S/CH4/h1H4", "inchi_key": "VNWKTOKETHGBQD-UHFFFAOYSA-N"} # Reference data for Li CIF test (test-004) — verifies oxidation state stripping (Li0+ -> Li) diff --git a/tests/integration/parsers/apps/nwchem/test_parser.py b/tests/integration/parsers/apps/nwchem/test_parser.py index d123c5e0..fb25ae90 100644 --- a/tests/integration/parsers/apps/nwchem/test_parser.py +++ b/tests/integration/parsers/apps/nwchem/test_parser.py @@ -71,9 +71,7 @@ def test_nwchem_thermal_correction_to_energy(self): self.assertAlmostEqual(self.parser.thermal_correction_to_energy(), THERMAL_CORRECTION_TO_ENERGY, places=2) def test_nwchem_thermal_correction_to_enthalpy(self): - self.assertAlmostEqual( - self.parser.thermal_correction_to_enthalpy(), THERMAL_CORRECTION_TO_ENTHALPY, places=2 - ) + self.assertAlmostEqual(self.parser.thermal_correction_to_enthalpy(), THERMAL_CORRECTION_TO_ENTHALPY, places=2) def test_nwchem_material_is_a_molecule_without_being_told(self): # Constructed WITHOUT is_non_periodic on purpose: rupy never passes it. diff --git a/tests/unit/parsers/test_nwchem_txt_parser.py b/tests/unit/parsers/test_nwchem_txt_parser.py index 58b45e5e..5afd2ac6 100644 --- a/tests/unit/parsers/test_nwchem_txt_parser.py +++ b/tests/unit/parsers/test_nwchem_txt_parser.py @@ -1,3 +1,4 @@ +import math import unittest from express.parsers.apps.nwchem.formats.txt import NwchemTXTParser @@ -116,6 +117,22 @@ def test_converts_a_block_printed_in_atomic_units(self): coordinates = self.parser.final_basis(text)["coordinates"] self.assertAlmostEqual(coordinates[0]["value"][2] - coordinates[1]["value"][2], 2.0, places=6) + def test_bond_length_of_an_atomic_units_block_is_physical(self): + """test-001's rows verbatim. Read as angstrom they give an O-H of 1.81 A; the conversion is + what makes them the 0.9572 A of a real water molecule, and the BASIS fixture is those.""" + text = geometry_block( + "a.u.", + "1.000000000", + [ + " 1 O 8.0000 0.00000000 0.00000000 0.22143053", + " 2 H 1.0000 0.00000000 1.43042809 -0.88572213", + " 3 H 1.0000 0.00000000 -1.43042809 -0.88572213", + ], + ) + coordinates = [c["value"] for c in self.parser.final_basis(text)["coordinates"]] + self.assertAlmostEqual(math.dist(coordinates[0], coordinates[1]), 0.9572, places=4) + self.assertAlmostEqual(math.dist(coordinates[0], coordinates[2]), 0.9572, places=4) + def test_centers_the_basis_inside_the_derived_cell(self): text = geometry_block("angstroms", "1.889725989", self.ANGSTROM_ROWS) edge = self.parser.final_lattice_vectors(text)["vectors"]["a"][0] @@ -142,13 +159,21 @@ def test_ignores_a_numeric_row_that_happens_to_fit_the_shape(self): def test_shared_cell_fits_a_relaxation_that_expands(self): # Sizing from the initial geometry alone would leave an expanded molecule outside the box, # which reads as extra fragments and corrupts the InChI. - text = geometry_block("angstroms", "1.889725989", [ - " 1 O 8.0000 0.00000000 0.00000000 0.20000000", - " 2 H 1.0000 0.00000000 0.00000000 -0.20000000", - ]) + geometry_block("angstroms", "1.889725989", [ - " 1 O 8.0000 0.00000000 0.00000000 2.50000000", - " 2 H 1.0000 0.00000000 0.00000000 -2.50000000", - ]) + text = geometry_block( + "angstroms", + "1.889725989", + [ + " 1 O 8.0000 0.00000000 0.00000000 0.20000000", + " 2 H 1.0000 0.00000000 0.00000000 -0.20000000", + ], + ) + geometry_block( + "angstroms", + "1.889725989", + [ + " 1 O 8.0000 0.00000000 0.00000000 2.50000000", + " 2 H 1.0000 0.00000000 0.00000000 -2.50000000", + ], + ) edge = self.parser.final_lattice_vectors(text)["vectors"]["a"][0] self.assertEqual(self.parser.initial_lattice_vectors(text), self.parser.final_lattice_vectors(text)) for basis in (self.parser.initial_basis(text), self.parser.final_basis(text)): diff --git a/tests/unit/properties/non_scalar/test_dielectric_tensor.py b/tests/unit/properties/non_scalar/test_dielectric_tensor.py index 5e5dcf7a..ce3576d2 100644 --- a/tests/unit/properties/non_scalar/test_dielectric_tensor.py +++ b/tests/unit/properties/non_scalar/test_dielectric_tensor.py @@ -36,72 +36,53 @@ def test_dielectric_tensor(self): property_ = DielectricTensor("dielectric_tensor", parser) self.assertDeepAlmostEqual(property_.serialize_and_validate(), DIELECTRIC_TENSOR) + DIELECTRIC_TENSOR = { "name": "dielectric_tensor", "values": [ { "part": "real", "spin": 0.5, - "frequencies": [ - 0.000000000, - 0.060120240, - 0.120240481, - 0.180360721 - ], + "frequencies": [0.000000000, 0.060120240, 0.120240481, 0.180360721], "components": [ [20.137876673, 20.137876704, 20.137849785], [20.143821034, 20.143821066, 20.143794147], [20.161680126, 20.161680158, 20.161653237], [20.191532277, 20.191532311, 20.191505388], - ] + ], }, { "part": "imaginary", "spin": 0.5, - "frequencies": [ - 0.000000000, - 0.060120240, - 0.120240481, - 0.180360721 - ], + "frequencies": [0.000000000, 0.060120240, 0.120240481, 0.180360721], "components": [ [20.137876673, 20.137876704, 20.137849785], [20.143821034, 20.143821066, 20.143794147], [20.161680126, 20.161680158, 20.161653237], [20.191532277, 20.191532311, 20.191505388], - ] + ], }, { "part": "real", "spin": -0.5, - "frequencies": [ - 0.000000000, - 0.060120240, - 0.120240481, - 0.180360721 - ], + "frequencies": [0.000000000, 0.060120240, 0.120240481, 0.180360721], "components": [ [20.137876673, 20.137876704, 20.137849785], [20.143821034, 20.143821066, 20.143794147], [20.161680126, 20.161680158, 20.161653237], [20.191532277, 20.191532311, 20.191505388], - ] + ], }, { "part": "imaginary", "spin": -0.5, - "frequencies": [ - 0.000000000, - 0.060120240, - 0.120240481, - 0.180360721 - ], + "frequencies": [0.000000000, 0.060120240, 0.120240481, 0.180360721], "components": [ [20.137876673, 20.137876704, 20.137849785], [20.143821034, 20.143821066, 20.143794147], [20.161680126, 20.161680158, 20.161653237], [20.191532277, 20.191532311, 20.191505388], - ] - } - ] + ], + }, + ], } diff --git a/tests/unit/properties/non_scalar/two_dimensional_plot/test_wavefunction_amplitude.py b/tests/unit/properties/non_scalar/two_dimensional_plot/test_wavefunction_amplitude.py index a05b3293..f2535d19 100644 --- a/tests/unit/properties/non_scalar/two_dimensional_plot/test_wavefunction_amplitude.py +++ b/tests/unit/properties/non_scalar/two_dimensional_plot/test_wavefunction_amplitude.py @@ -3,15 +3,12 @@ RAW_DATA_ALAT = [ [0.0, 0.0050251256, 0.0100502513, 0.0150753769, 0.0201005025], - [0.0000322091, 0.0000072134, -0.0000218274, -0.0000540398, -0.0000883573] + [0.0000322091, 0.0000072134, -0.0000218274, -0.0000540398, -0.0000883573], ] ALAT_ANGSTROM = 10.0 -CONVERTED_DATA_ANGSTROMS = [ - [x * ALAT_ANGSTROM for x in RAW_DATA_ALAT[0]], - RAW_DATA_ALAT[1] -] +CONVERTED_DATA_ANGSTROMS = [[x * ALAT_ANGSTROM for x in RAW_DATA_ALAT[0]], RAW_DATA_ALAT[1]] EXPECTED = { "name": "wavefunction_amplitude", @@ -33,4 +30,3 @@ def test_wavefunction_amplitude(self): parser = self.get_mocked_parser("wavefunction_amplitude", CONVERTED_DATA_ANGSTROMS) property_ = WavefunctionAmplitude("wavefunction_amplitude", parser) self.assertDeepAlmostEqual(property_.serialize_and_validate(), EXPECTED) - diff --git a/tests/unit/properties/scalar/test_defect_formation_energy.py b/tests/unit/properties/scalar/test_defect_formation_energy.py index 8b7da432..94484159 100644 --- a/tests/unit/properties/scalar/test_defect_formation_energy.py +++ b/tests/unit/properties/scalar/test_defect_formation_energy.py @@ -12,7 +12,5 @@ def tearDown(self): super().tearDown() def test_defect_formation_energy(self): - property_ = ScalarPropertyFromContext( - "defect_formation_energy", None, value=DEFECT_FORMATION_ENERGY["value"] - ) + property_ = ScalarPropertyFromContext("defect_formation_energy", None, value=DEFECT_FORMATION_ENERGY["value"]) self.assertDeepAlmostEqual(property_.serialize_and_validate(), DEFECT_FORMATION_ENERGY) diff --git a/tests/unit/properties/scalar/test_formation_energy.py b/tests/unit/properties/scalar/test_formation_energy.py index f35ef34d..fc7c1fa4 100644 --- a/tests/unit/properties/scalar/test_formation_energy.py +++ b/tests/unit/properties/scalar/test_formation_energy.py @@ -12,6 +12,6 @@ def tearDown(self): super().tearDown() def test_formation_energy(self): - parser = self.get_mocked_parser("formation_energy", FORMATION_ENERGY["value"]) # noqa : F841 + parser = self.get_mocked_parser("formation_energy", FORMATION_ENERGY["value"]) # noqa : F841 property_ = ScalarPropertyFromContext("formation_energy", None, value=FORMATION_ENERGY["value"]) self.assertDeepAlmostEqual(property_.serialize_and_validate(), FORMATION_ENERGY) From 5c7209284627b6d225ba2d9bff05984b3f8afb3f Mon Sep 17 00:00:00 2001 From: VsevolodX Date: Wed, 26 Aug 2026 09:38:43 -0700 Subject: [PATCH 08/16] SOF-8005: give the properties layer the molecular cell CEO review on #165: parsers parse text into data structures; the cell and the centering are a separate thing, one layer up. Material.__init__ already post-processes parser output, so box_molecule lands beside it. Inert until the next commit: it fires only when the parser returns no lattice, and today every parser returns one. Co-Authored-By: Claude Fable 5 --- express/properties/material.py | 5 +++++ express/properties/utils.py | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/express/properties/material.py b/express/properties/material.py index 23752fcb..23f12dfc 100644 --- a/express/properties/material.py +++ b/express/properties/material.py @@ -13,6 +13,7 @@ from express.properties.scalar.volume import Volume from express.properties.structural.inchi import Inchi from express.properties.structural.inchi_key import InchiKey +from express.properties.utils import box_molecule class Material(BaseProperty): @@ -36,6 +37,8 @@ def __init__(self, name, parser, *args, **kwargs): else: basis = self.parser.initial_basis() lattice = self.parser.initial_lattice_vectors() + if self.is_non_periodic and lattice is None: + lattice, basis = box_molecule(basis, [basis, self.parser.final_basis()]) structure_string = lattice_basis_to_poscar(lattice, basis) if kwargs.get("is_final_structure"): @@ -45,6 +48,8 @@ def __init__(self, name, parser, *args, **kwargs): else: basis = self.parser.final_basis() lattice = self.parser.final_lattice_vectors() + if self.is_non_periodic and lattice is None: + lattice, basis = box_molecule(basis, [self.parser.initial_basis(), basis]) structure_string = lattice_basis_to_poscar(lattice, basis) if self.is_non_periodic: diff --git a/express/properties/utils.py b/express/properties/utils.py index d91d4892..5d5bb7d3 100644 --- a/express/properties/utils.py +++ b/express/properties/utils.py @@ -1,3 +1,7 @@ +from mat3ra.made.tools.convert.utils import calculate_padded_cell_simple_cubic +from mat3ra.made.utils import get_center_of_coordinates + + def eigenvalues(eigenvalues_at_kpoints, kpoint_index=0, spin_index=0): """ Returns eigenvalues for a given kpoint and spin. @@ -27,3 +31,32 @@ def to_array_with_ids(array): list """ return [{"id": index, "value": value} for index, value in enumerate(array)] + + +def box_molecule(basis, bases): + """ + Puts a molecule into the cell its application does not print: made's simple-cubic padding + convention, the same one that gives every non-periodic material on the platform its box. + + One cell for every basis in `bases`, so the structures of one calculation stay comparable: an + optimization moves atoms inside a fixed box rather than resizing it. Sized to whichever geometry + needs the most room, and the basis is then centered in it -- printed coordinates straddle the + origin, and atoms outside the box read as extra fragments and corrupt the InChI. + + Args: + basis (dict): the basis to center. + bases (list): every basis the cell has to hold. + + Returns: + tuple[dict, dict]: lattice vectors and the centered basis. + """ + coordinates = [coordinate["value"] for coordinate in basis["coordinates"]] + edge = max( + calculate_padded_cell_simple_cubic([point["value"] for point in other["coordinates"]])[0][0] for other in bases + ) + center = get_center_of_coordinates(coordinates) + centered = [[x - center[axis] + edge / 2 for axis, x in enumerate(coordinate)] for coordinate in coordinates] + return ( + {"vectors": {"a": [edge, 0.0, 0.0], "b": [0.0, edge, 0.0], "c": [0.0, 0.0, edge], "alat": 1}}, + dict(basis, coordinates=to_array_with_ids(centered)), + ) From 1290ced9124978ca299b0a24a3819d280bcd2303 Mon Sep 17 00:00:00 2001 From: VsevolodX Date: Wed, 26 Aug 2026 09:39:44 -0700 Subject: [PATCH 09/16] SOF-8005: make the nwchem parser parse-only geometry_blocks() converts units and nothing else; basis(text, index) picks a block by id, 0 initial and -1 final, which is the "extract all structure entries and then choose by id" comment. The cell derivation and the centering are gone, and with them the mat3ra-made import. Both lattice delegates are deleted rather than reworded. They returned the same invented cell, and IonicDataMixin declares them `pass` without ABCMeta, so unimplemented is None - which is what VASP already relies on by omitting initial_*. references.py BASIS goes back to plain converted coordinates. It held centered ones, which is what "Seems suspicious" was pointing at: a fixture recording that the parser had moved the atoms. Co-Authored-By: Claude Fable 5 --- express/parsers/apps/nwchem/formats/txt.py | 97 +++---------------- express/parsers/apps/nwchem/parser.py | 24 +---- tests/fixtures/nwchem/references.py | 30 +++--- .../parsers/apps/nwchem/test_parser.py | 5 - tests/unit/parsers/test_nwchem_txt_parser.py | 47 ++------- 5 files changed, 34 insertions(+), 169 deletions(-) diff --git a/express/parsers/apps/nwchem/formats/txt.py b/express/parsers/apps/nwchem/formats/txt.py index d7766cb2..253d60a1 100644 --- a/express/parsers/apps/nwchem/formats/txt.py +++ b/express/parsers/apps/nwchem/formats/txt.py @@ -1,6 +1,3 @@ -from mat3ra.made.tools.convert.utils import calculate_padded_cell_simple_cubic -from mat3ra.made.utils import get_center_of_coordinates - from express.parsers.settings import Constant from express.parsers.apps.nwchem import settings from express.parsers.formats.txt import BaseTXTParser @@ -15,33 +12,16 @@ class NwchemTXTParser(BaseTXTParser): def __init__(self, work_dir): super(NwchemTXTParser, self).__init__(work_dir) - def _geometry_block(self, text, last): - """ - Extracts one "Output coordinates" block as coordinates in angstrom. - - A geometry optimization prints one block per step; a single-point run prints exactly one, so - the first and the last block coincide and the initial and final structures are equal. The - block is printed in whichever units the input declared, and the header carries the factor - converting them to a.u., so both `units angstrom` and `units au` runs are read correctly. - Returns empty lists when the blocks disagree -- see `_geometry_blocks`. - - Args: - text (str): text to extract data from. - last (bool): whether to read the last block instead of the first. - - Returns: - tuple[list[str], list[list[float]]]: elements and coordinates in angstrom. - """ - blocks = self._geometry_blocks(text) - return blocks[-1 if last else 0] if blocks else ([], []) - - def _geometry_blocks(self, text): + def geometry_blocks(self, text): """ Extracts every "Output coordinates" block, or nothing at all if they disagree on which atoms are present. A log truncated mid-table parses as a shorter molecule, and the callers below cannot tell that from a real one -- rupy would publish the fragment as `final_structure`, formula and InChI included. + A block is printed in whichever units the input declared, and its header carries the factor + converting them to a.u., so both `units angstrom` and `units au` runs are read correctly. + Args: text (str): text to extract data from. @@ -62,14 +42,14 @@ def _geometry_blocks(self, text): return [] return blocks - def _basis(self, text, last): + def basis(self, text, index): """ - Extracts a basis, centered inside the one cell `_lattice_vectors` derives from both - blocks. NWChem's coordinates straddle the origin and would otherwise sit outside the box. + Extracts the geometry block at the given index. An optimization prints one block per step; + a single-point run prints exactly one, so index 0 and index -1 then coincide. Args: text (str): text to extract data from. - last (bool): whether to read the last block instead of the first. + index (int): position of the block among those printed. Returns: dict @@ -78,69 +58,20 @@ def _basis(self, text, last): { 'units': 'angstrom', 'elements': [{'id': 0, 'value': 'O'}, {'id': 1, 'value': 'H'}], - 'coordinates': [{'id': 0, 'value': [2.86, 2.86, 3.60]}, {'id': 1, 'value': [1.43, 2.86, 2.49]}] + 'coordinates': [{'id': 0, 'value': [0.0, 0.0, 0.11]}, {'id': 1, 'value': [0.0, 0.75, -0.46]}] } """ - elements, coordinates = self._geometry_block(text, last) - if not elements: + blocks = self.geometry_blocks(text) + if not blocks: return None - # Take the edge from _lattice_vectors rather than deriving a second cell here, so the basis - # is centered in the very box that ships with it. - center = get_center_of_coordinates(coordinates) - box_center = self._lattice_vectors(text)["vectors"]["a"][0] / 2 + elements, coordinates = blocks[index] return { "units": "angstrom", - "elements": [{"id": index, "value": value} for index, value in enumerate(elements)], - "coordinates": [ - {"id": index, "value": [x - center[axis] + box_center for axis, x in enumerate(coordinate)]} - for index, coordinate in enumerate(coordinates) - ], + "elements": [{"id": idx, "value": value} for idx, value in enumerate(elements)], + "coordinates": [{"id": idx, "value": coordinate} for idx, coordinate in enumerate(coordinates)], } - def _lattice_vectors(self, text): - """ - Derives a cell for a molecule, which NWChem does not print: made's simple-cubic padding - convention, the same one that gives every non-periodic material on the platform its box. - - One cell for both structures, so they are comparable: an optimization moves atoms inside a - fixed box rather than resizing it, and initial and final differ only where the atoms went. - Sized to whichever geometry needs more room. Containment is not the reason -- a cell derived - per structure always contains its own atoms; it is centering a basis in a cell derived from - the other block that puts atoms outside, which reads as extra fragments and corrupts the - InChI. - - Args: - text (str): text to extract data from. - - Returns: - dict - - Example: - {'vectors': {'a': [5.72, 0.0, 0.0], 'b': [0.0, 5.72, 0.0], 'c': [0.0, 0.0, 5.72], 'alat': 1}} - """ - edges = [ - calculate_padded_cell_simple_cubic(coordinates)[0][0] - for _, coordinates in self._geometry_blocks(text) - if coordinates - ] - if not edges: - return None - - edge = max(edges) - return {"vectors": {"a": [edge, 0.0, 0.0], "b": [0.0, edge, 0.0], "c": [0.0, 0.0, edge], "alat": 1}} - - def initial_basis(self, text): - return self._basis(text, last=False) - - def final_basis(self, text): - return self._basis(text, last=True) - - def initial_lattice_vectors(self, text): - return self._lattice_vectors(text) - - final_lattice_vectors = initial_lattice_vectors - def eigenvalues_at_vectors(self, text): """ Extracts eigenvalues at molecular orbitals (vectors). Geometry optimizations print one diff --git a/express/parsers/apps/nwchem/parser.py b/express/parsers/apps/nwchem/parser.py index 9799cae1..57f4185d 100644 --- a/express/parsers/apps/nwchem/parser.py +++ b/express/parsers/apps/nwchem/parser.py @@ -58,7 +58,7 @@ def initial_basis(self): Reference: func: express.parsers.mixins.ionic.IonicDataMixin.initial_basis """ - return self.txt_parser.initial_basis(self._get_file_content(self.stdout_file)) + return self.txt_parser.basis(self._get_file_content(self.stdout_file), 0) def final_basis(self): """ @@ -67,27 +67,7 @@ def final_basis(self): Reference: func: express.parsers.mixins.ionic.IonicDataMixin.final_basis """ - return self.txt_parser.final_basis(self._get_file_content(self.stdout_file)) - - def initial_lattice_vectors(self): - """ - Returns initial lattice vectors. - - Reference: - func: express.parsers.mixins.ionic.IonicDataMixin.initial_lattice_vectors - NWChem does not print a cell for a molecule; one is derived per made's convention. - """ - return self.txt_parser.initial_lattice_vectors(self._get_file_content(self.stdout_file)) - - def final_lattice_vectors(self): - """ - Returns final lattice vectors. - - Reference: - func: express.parsers.mixins.ionic.IonicDataMixin.final_lattice_vectors - NWChem does not print a cell for a molecule; one is derived per made's convention. - """ - return self.txt_parser.final_lattice_vectors(self._get_file_content(self.stdout_file)) + return self.txt_parser.basis(self._get_file_content(self.stdout_file), -1) def eigenvalues_at_vectors(self): """ diff --git a/tests/fixtures/nwchem/references.py b/tests/fixtures/nwchem/references.py index 091ef14b..7f6b2fee 100644 --- a/tests/fixtures/nwchem/references.py +++ b/tests/fixtures/nwchem/references.py @@ -39,42 +39,36 @@ "nuclear_repulsion": {"name": "nuclear_repulsion", "value": 250.20815670232923}, } -# test-001 declares `units au`, so these are its printed coordinates converted to angstrom and then -# centered in the derived cell. Read as angstrom instead they give an O-H of 1.81 A, which is what +# test-001 declares `units au`, so these are its printed coordinates converted to angstrom. Read as +# angstrom instead they give an O-H of 1.81 A, which is what # test_bond_length_of_an_atomic_units_block_is_physical guards against. BASIS = { "units": "angstrom", "elements": [{"id": 0, "value": "O"}, {"id": 1, "value": "H"}, {"id": 2, "value": "H"}], "coordinates": [ - {"id": 0, "value": [1.51390003, 1.51390003, 1.90448670]}, - {"id": 1, "value": [1.51390003, 2.27085004, 1.31860669]}, - {"id": 2, "value": [1.51390003, 0.75695001, 1.31860669]}, + {"id": 0, "value": [0.0, 0.0, 0.11717600]}, + {"id": 1, "value": [0.0, 0.75695001, -0.46870401]}, + {"id": 2, "value": [0.0, -0.75695001, -0.46870401]}, ], } -LATTICE_VECTORS = { - "vectors": {"a": [3.02780005, 0.0, 0.0], "b": [0.0, 3.02780005, 0.0], "c": [0.0, 0.0, 3.02780005], "alat": 1} -} -# test-002 optimizes, so its first and last blocks differ — and so do the cells derived from them. +# test-002 optimizes, so its first and last blocks differ. # 6-31G* geometry: O-H 0.96866 A after relaxation, not the 6-31G 0.9758 A the Cypress feature pins. INITIAL_BASIS_MULTISTEP = { "units": "angstrom", "elements": [{"id": 0, "value": "O"}, {"id": 1, "value": "H"}, {"id": 2, "value": "H"}], "coordinates": [ - {"id": 0, "value": [2.86085618, 2.86085618, 3.59895795]}, - {"id": 1, "value": [1.43042809, 2.86085618, 2.49180529]}, - {"id": 2, "value": [4.29128427, 2.86085618, 2.49180529]}, + {"id": 0, "value": [0.0, 0.0, 0.22143053]}, + {"id": 1, "value": [-1.43042811, 0.0, -0.88572214]}, + {"id": 2, "value": [1.43042811, 0.0, -0.88572214]}, ], } FINAL_BASIS_MULTISTEP = { "units": "angstrom", "elements": [{"id": 0, "value": "O"}, {"id": 1, "value": "H"}, {"id": 2, "value": "H"}], "coordinates": [ - {"id": 0, "value": [2.86085618, 2.86085618, 3.25903001]}, - {"id": 1, "value": [2.09824137, 2.86085618, 2.66176926]}, - {"id": 2, "value": [3.62347099, 2.86085618, 2.66176926]}, + {"id": 0, "value": [0.0, 0.0, -0.11849741]}, + {"id": 1, "value": [-0.76261482, 0.0, -0.71575817]}, + {"id": 2, "value": [0.76261482, 0.0, -0.71575817]}, ], } -LATTICE_VECTORS_MULTISTEP = { - "vectors": {"a": [5.72171236, 0.0, 0.0], "b": [0.0, 5.72171236, 0.0], "c": [0.0, 0.0, 5.72171236], "alat": 1} -} diff --git a/tests/integration/parsers/apps/nwchem/test_parser.py b/tests/integration/parsers/apps/nwchem/test_parser.py index fb25ae90..af587d47 100644 --- a/tests/integration/parsers/apps/nwchem/test_parser.py +++ b/tests/integration/parsers/apps/nwchem/test_parser.py @@ -51,15 +51,10 @@ def test_nwchem_lumo_energy_multistep(self): def test_nwchem_structures_of_single_point(self): self.assertDeepAlmostEqual(self.parser.initial_basis(), BASIS, places=6) self.assertDeepAlmostEqual(self.parser.final_basis(), BASIS, places=6) - self.assertDeepAlmostEqual(self.parser.initial_lattice_vectors(), LATTICE_VECTORS, places=6) - self.assertDeepAlmostEqual(self.parser.final_lattice_vectors(), LATTICE_VECTORS, places=6) def test_nwchem_structures_of_optimization(self): self.assertDeepAlmostEqual(self.parser.initial_basis(), INITIAL_BASIS_MULTISTEP, places=6) self.assertDeepAlmostEqual(self.parser.final_basis(), FINAL_BASIS_MULTISTEP, places=6) - self.assertDeepAlmostEqual(self.parser.initial_lattice_vectors(), LATTICE_VECTORS_MULTISTEP, places=6) - self.assertDeepAlmostEqual(self.parser.final_lattice_vectors(), LATTICE_VECTORS_MULTISTEP, places=6) - self.assertEqual(self.parser.initial_lattice_vectors(), self.parser.final_lattice_vectors()) def test_nwchem_total_energy_contributions(self): self.assertDeepAlmostEqual(self.parser.total_energy_contributions(), TOTAL_ENERGY_CONTRIBUTION, places=2) diff --git a/tests/unit/parsers/test_nwchem_txt_parser.py b/tests/unit/parsers/test_nwchem_txt_parser.py index 5afd2ac6..d3adbe57 100644 --- a/tests/unit/parsers/test_nwchem_txt_parser.py +++ b/tests/unit/parsers/test_nwchem_txt_parser.py @@ -100,8 +100,8 @@ def test_reads_first_and_last_block_of_an_optimization(self): " 2 H 1.0000 0.00000000 0.00000000 -0.50000000", ], ) - initial = self.parser.initial_basis(text)["coordinates"] - final = self.parser.final_basis(text)["coordinates"] + initial = self.parser.basis(text, 0)["coordinates"] + final = self.parser.basis(text, -1)["coordinates"] self.assertAlmostEqual(initial[0]["value"][2] - initial[1]["value"][2], 2.0) self.assertAlmostEqual(final[0]["value"][2] - final[1]["value"][2], 1.0) @@ -114,7 +114,7 @@ def test_converts_a_block_printed_in_atomic_units(self): " 2 H 1.0000 0.00000000 0.00000000 -1.88972599", ], ) - coordinates = self.parser.final_basis(text)["coordinates"] + coordinates = self.parser.basis(text, -1)["coordinates"] self.assertAlmostEqual(coordinates[0]["value"][2] - coordinates[1]["value"][2], 2.0, places=6) def test_bond_length_of_an_atomic_units_block_is_physical(self): @@ -129,21 +129,12 @@ def test_bond_length_of_an_atomic_units_block_is_physical(self): " 3 H 1.0000 0.00000000 -1.43042809 -0.88572213", ], ) - coordinates = [c["value"] for c in self.parser.final_basis(text)["coordinates"]] + coordinates = [c["value"] for c in self.parser.basis(text, -1)["coordinates"]] self.assertAlmostEqual(math.dist(coordinates[0], coordinates[1]), 0.9572, places=4) self.assertAlmostEqual(math.dist(coordinates[0], coordinates[2]), 0.9572, places=4) - def test_centers_the_basis_inside_the_derived_cell(self): - text = geometry_block("angstroms", "1.889725989", self.ANGSTROM_ROWS) - edge = self.parser.final_lattice_vectors(text)["vectors"]["a"][0] - for coordinate in self.parser.final_basis(text)["coordinates"]: - for value in coordinate["value"]: - self.assertGreaterEqual(value, 0.0) - self.assertLessEqual(value, edge) - def test_returns_nothing_without_a_geometry_block(self): - self.assertIsNone(self.parser.final_basis(" Total DFT energy = -76.4\n")) - self.assertIsNone(self.parser.final_lattice_vectors(" Total DFT energy = -76.4\n")) + self.assertIsNone(self.parser.basis(" Total DFT energy = -76.4\n", -1)) def test_ignores_a_numeric_row_that_happens_to_fit_the_shape(self): # A row of bare numbers satisfies the column shape but has no element symbol. It must be @@ -153,31 +144,5 @@ def test_ignores_a_numeric_row_that_happens_to_fit_the_shape(self): "1.889725989", self.ANGSTROM_ROWS + [" 3 1.0000 2.0000 3.0000 4.0000 5.0000"], ) - basis = self.parser.final_basis(text) + basis = self.parser.basis(text, -1) self.assertEqual([e["value"] for e in basis["elements"]], ["O", "H"]) - - def test_shared_cell_fits_a_relaxation_that_expands(self): - # Sizing from the initial geometry alone would leave an expanded molecule outside the box, - # which reads as extra fragments and corrupts the InChI. - text = geometry_block( - "angstroms", - "1.889725989", - [ - " 1 O 8.0000 0.00000000 0.00000000 0.20000000", - " 2 H 1.0000 0.00000000 0.00000000 -0.20000000", - ], - ) + geometry_block( - "angstroms", - "1.889725989", - [ - " 1 O 8.0000 0.00000000 0.00000000 2.50000000", - " 2 H 1.0000 0.00000000 0.00000000 -2.50000000", - ], - ) - edge = self.parser.final_lattice_vectors(text)["vectors"]["a"][0] - self.assertEqual(self.parser.initial_lattice_vectors(text), self.parser.final_lattice_vectors(text)) - for basis in (self.parser.initial_basis(text), self.parser.final_basis(text)): - for coordinate in basis["coordinates"]: - for value in coordinate["value"]: - self.assertGreaterEqual(value, 0.0) - self.assertLessEqual(value, edge) From 931386686c52555a9259eec62de2ff5a043e263c Mon Sep 17 00:00:00 2001 From: VsevolodX Date: Wed, 26 Aug 2026 09:40:06 -0700 Subject: [PATCH 10/16] SOF-8005: pin the cell and coordinates the cluster published The relocation is only worth anything if it changed no output, so assert the numbers the live job produced: edge 3.163848897 and O at [1.337365565, 1.554783897, 1.291226523], from web-app's final-structure-nwchem-optimized.json fixture. test-003 is that geometry translated to the origin, so the padding and the centering are what have to put it back - fed its own published coordinates the pipeline would be a fixed point and a missing centering step would pass. Verified against a488316 rather than asserted: the full serialized material for test-001, test-002 and test-003, initial and final, is byte-identical before and after the move. Co-Authored-By: Claude Fable 5 --- .gitignore | 1 + tests/fixtures/nwchem/references.py | 10 ++++++++++ .../nwchem/test-003/nwchem-relaxation.log | 11 +++++++++++ .../parsers/apps/nwchem/test_parser.py | 15 +++++++++++++++ tests/manifest.yaml | 8 ++++++++ 5 files changed, 45 insertions(+) create mode 100644 tests/fixtures/nwchem/test-003/nwchem-relaxation.log diff --git a/.gitignore b/.gitignore index faaf1f26..9c27f758 100644 --- a/.gitignore +++ b/.gitignore @@ -63,6 +63,7 @@ coverage.xml # Django stuff: *.log !tests/fixtures/nwchem/test-002/*.log +!tests/fixtures/nwchem/test-003/*.log local_settings.py # Flask stuff: diff --git a/tests/fixtures/nwchem/references.py b/tests/fixtures/nwchem/references.py index 7f6b2fee..45a6030c 100644 --- a/tests/fixtures/nwchem/references.py +++ b/tests/fixtures/nwchem/references.py @@ -72,3 +72,13 @@ {"id": 2, "value": [0.76261482, 0.0, -0.71575817]}, ], } + +# What express published for the relaxed H2O of the live cluster job (B3LYP/6-31G), pinned from +# web-app cypress/fixtures/properties/final-structure-nwchem-optimized.json. test-003 holds that +# geometry translated to the origin, so the box and the centering are what have to put it back. +RELAXED_CELL_EDGE = 3.163848897 +RELAXED_COORDINATES = [ + [1.337365565, 1.554783897, 1.291226523], + [1.738223968, 0.806541518, 1.772181243], + [1.670183813, 2.38444793, 1.682365581], +] diff --git a/tests/fixtures/nwchem/test-003/nwchem-relaxation.log b/tests/fixtures/nwchem/test-003/nwchem-relaxation.log new file mode 100644 index 00000000..20a4a4b3 --- /dev/null +++ b/tests/fixtures/nwchem/test-003/nwchem-relaxation.log @@ -0,0 +1,11 @@ + Northwest Computational Chemistry Package (NWChem) 7.0.2 + + Output coordinates in angstroms (scale by 1.889725989 to convert to a.u.) + + No. Tag Charge X Y Z + ---- ---------------- ---------- -------------- -------------- -------------- + 1 O 8.0000 -0.24455888 -0.02714055 -0.29069793 + 2 H 1.0000 0.15629952 -0.77538293 0.19025679 + 3 H 1.0000 0.08825936 0.80252348 0.10044113 + + Atomic Mass diff --git a/tests/integration/parsers/apps/nwchem/test_parser.py b/tests/integration/parsers/apps/nwchem/test_parser.py index af587d47..ae3c577d 100644 --- a/tests/integration/parsers/apps/nwchem/test_parser.py +++ b/tests/integration/parsers/apps/nwchem/test_parser.py @@ -78,3 +78,18 @@ def test_nwchem_material_is_a_molecule_without_being_told(self): self.assertIn("inchi_key", derived) self.assertNotIn("volume", derived) self.assertNotIn("density", derived) + + def test_nwchem_material_shares_one_cell_between_structures(self): + initial = Material("material", self.parser, is_initial_structure=True).serialize_and_validate() + final = Material("material", self.parser, is_final_structure=True).serialize_and_validate() + self.assertEqual(initial["lattice"], final["lattice"]) + + def test_nwchem_material_of_a_relaxed_molecule(self): + """The cell and the coordinates the live cluster job published, which express serializes in + crystal units, so they come back multiplied by the edge.""" + material = Material("material", self.parser, is_final_structure=True).serialize_and_validate() + edge = material["lattice"]["a"] + self.assertAlmostEqual(edge, RELAXED_CELL_EDGE, places=6) + self.assertDeepAlmostEqual( + [[edge * x for x in c["value"]] for c in material["basis"]["coordinates"]], RELAXED_COORDINATES, places=6 + ) diff --git a/tests/manifest.yaml b/tests/manifest.yaml index 9c10141c..5b62dfd4 100644 --- a/tests/manifest.yaml +++ b/tests/manifest.yaml @@ -268,3 +268,11 @@ test_espresso_hubbard_v_nn: test_nwchem_material_is_a_molecule_without_being_told: workDir: fixtures/nwchem/test-002 stdoutFile: fixtures/nwchem/test-002/nwchem-frequency.log + +test_nwchem_material_shares_one_cell_between_structures: + workDir: fixtures/nwchem/test-002 + stdoutFile: fixtures/nwchem/test-002/nwchem-frequency.log + +test_nwchem_material_of_a_relaxed_molecule: + workDir: fixtures/nwchem/test-003 + stdoutFile: fixtures/nwchem/test-003/nwchem-relaxation.log From 2b60f711b00aa048bd60dbbfd3d452397f5f85b8 Mon Sep 17 00:00:00 2001 From: VsevolodX Date: Wed, 26 Aug 2026 09:48:00 -0700 Subject: [PATCH 11/16] SOF-8005: pin test-002's own numbers instead of a reconstructed log Reverses the fixture half of 9313866. test-003 was the relaxed cluster geometry re-emitted in NWChem's column format - the only "NWChem output" in the repo that no NWChem ever printed, and opaque fixture data is what triggered this review in the first place. The relocation invariant does not need it: the full serialized material for test-001 and test-002, initial and final, is byte-identical to a488316, and the cluster's own numbers stay pinned where they belong, in web-app's final-structure-nwchem-optimized.json and the feature's HOMO/LUMO assertions. So the assertions move onto test-002, which is a real geometry optimization. Mutation strength is unchanged or better: reverting the centering fails one test, reverting the per-block sizing now fails two (test-002's final block alone gives 3.05 A, not 5.72), reverting the material.py hook fails three. Co-Authored-By: Claude Fable 5 --- .gitignore | 1 - tests/fixtures/nwchem/references.py | 16 ++++++++-------- .../nwchem/test-003/nwchem-relaxation.log | 11 ----------- .../parsers/apps/nwchem/test_parser.py | 7 ++----- tests/manifest.yaml | 4 ++-- 5 files changed, 12 insertions(+), 27 deletions(-) delete mode 100644 tests/fixtures/nwchem/test-003/nwchem-relaxation.log diff --git a/.gitignore b/.gitignore index 9c27f758..faaf1f26 100644 --- a/.gitignore +++ b/.gitignore @@ -63,7 +63,6 @@ coverage.xml # Django stuff: *.log !tests/fixtures/nwchem/test-002/*.log -!tests/fixtures/nwchem/test-003/*.log local_settings.py # Flask stuff: diff --git a/tests/fixtures/nwchem/references.py b/tests/fixtures/nwchem/references.py index 45a6030c..a16b161c 100644 --- a/tests/fixtures/nwchem/references.py +++ b/tests/fixtures/nwchem/references.py @@ -73,12 +73,12 @@ ], } -# What express published for the relaxed H2O of the live cluster job (B3LYP/6-31G), pinned from -# web-app cypress/fixtures/properties/final-structure-nwchem-optimized.json. test-003 holds that -# geometry translated to the origin, so the box and the centering are what have to put it back. -RELAXED_CELL_EDGE = 3.163848897 -RELAXED_COORDINATES = [ - [1.337365565, 1.554783897, 1.291226523], - [1.738223968, 0.806541518, 1.772181243], - [1.670183813, 2.38444793, 1.682365581], +# What express serializes for test-002's final block: made's cubic padding sized to fit both +# structures, and the basis centered in it. Taken from a488316, where the parser did that itself -- +# these are the numbers the move into the properties layer had to leave alone. +FINAL_CELL_EDGE_MULTISTEP = 5.721712 +FINAL_CRYSTAL_COORDINATES_MULTISTEP = [ + [0.5, 0.5, 0.569589977], + [0.366715633, 0.5, 0.465205011], + [0.633284367, 0.5, 0.465205011], ] diff --git a/tests/fixtures/nwchem/test-003/nwchem-relaxation.log b/tests/fixtures/nwchem/test-003/nwchem-relaxation.log deleted file mode 100644 index 20a4a4b3..00000000 --- a/tests/fixtures/nwchem/test-003/nwchem-relaxation.log +++ /dev/null @@ -1,11 +0,0 @@ - Northwest Computational Chemistry Package (NWChem) 7.0.2 - - Output coordinates in angstroms (scale by 1.889725989 to convert to a.u.) - - No. Tag Charge X Y Z - ---- ---------------- ---------- -------------- -------------- -------------- - 1 O 8.0000 -0.24455888 -0.02714055 -0.29069793 - 2 H 1.0000 0.15629952 -0.77538293 0.19025679 - 3 H 1.0000 0.08825936 0.80252348 0.10044113 - - Atomic Mass diff --git a/tests/integration/parsers/apps/nwchem/test_parser.py b/tests/integration/parsers/apps/nwchem/test_parser.py index ae3c577d..918366b2 100644 --- a/tests/integration/parsers/apps/nwchem/test_parser.py +++ b/tests/integration/parsers/apps/nwchem/test_parser.py @@ -85,11 +85,8 @@ def test_nwchem_material_shares_one_cell_between_structures(self): self.assertEqual(initial["lattice"], final["lattice"]) def test_nwchem_material_of_a_relaxed_molecule(self): - """The cell and the coordinates the live cluster job published, which express serializes in - crystal units, so they come back multiplied by the edge.""" material = Material("material", self.parser, is_final_structure=True).serialize_and_validate() - edge = material["lattice"]["a"] - self.assertAlmostEqual(edge, RELAXED_CELL_EDGE, places=6) + self.assertAlmostEqual(material["lattice"]["a"], FINAL_CELL_EDGE_MULTISTEP, places=6) self.assertDeepAlmostEqual( - [[edge * x for x in c["value"]] for c in material["basis"]["coordinates"]], RELAXED_COORDINATES, places=6 + [c["value"] for c in material["basis"]["coordinates"]], FINAL_CRYSTAL_COORDINATES_MULTISTEP, places=6 ) diff --git a/tests/manifest.yaml b/tests/manifest.yaml index 5b62dfd4..add5f83d 100644 --- a/tests/manifest.yaml +++ b/tests/manifest.yaml @@ -274,5 +274,5 @@ test_nwchem_material_shares_one_cell_between_structures: stdoutFile: fixtures/nwchem/test-002/nwchem-frequency.log test_nwchem_material_of_a_relaxed_molecule: - workDir: fixtures/nwchem/test-003 - stdoutFile: fixtures/nwchem/test-003/nwchem-relaxation.log + workDir: fixtures/nwchem/test-002 + stdoutFile: fixtures/nwchem/test-002/nwchem-frequency.log From 645d36825c971ef8f6ff6e89105e1b2024acd35d Mon Sep 17 00:00:00 2001 From: VsevolodX Date: Wed, 26 Aug 2026 10:11:18 -0700 Subject: [PATCH 12/16] SOF-8005: test box_molecule, including the direction no fixture covers TB-TEST-1. The restructure deleted test_shared_cell_fits_a_relaxation_that _expands and replaced nothing, and the reviewer showed what that cost: change material.py's initial branch to box_molecule(basis, [basis]) and the whole suite still passes. test-002's initial block is the larger of its two, so its padded edge already IS the shared edge - the expanding direction, the one that leaves atoms outside the box and corrupts the InChI, is unexercisable by any committed fixture. tests/unit/properties/test_utils.py covers it with synthetic bases: the cell is sized to the largest structure, every structure is centred inside the shared cell, and Material hands over every parsed basis rather than only the one it is centring. That last one is what fails under the reviewer's mutation. The three Material tests move out of the nwchem parser module and in beside the other Material tests; the manifest is keyed on test name, so the move is free. Three rather than the two asked for: leaving the third behind would have kept a Material test in a parser module for no reason. Co-Authored-By: Claude Fable 5 --- .../parsers/apps/nwchem/test_parser.py | 24 -------- tests/integration/properties/test_material.py | 29 ++++++++++ tests/unit/properties/test_utils.py | 58 +++++++++++++++++++ 3 files changed, 87 insertions(+), 24 deletions(-) create mode 100644 tests/unit/properties/test_utils.py diff --git a/tests/integration/parsers/apps/nwchem/test_parser.py b/tests/integration/parsers/apps/nwchem/test_parser.py index 918366b2..777d5d8a 100644 --- a/tests/integration/parsers/apps/nwchem/test_parser.py +++ b/tests/integration/parsers/apps/nwchem/test_parser.py @@ -1,6 +1,5 @@ # ruff: noqa: F403,F405 from express.parsers.apps.nwchem.parser import NwchemParser -from express.properties.material import Material from tests.fixtures.nwchem.references import * from tests.integration import IntegrationTestBase @@ -67,26 +66,3 @@ def test_nwchem_thermal_correction_to_energy(self): def test_nwchem_thermal_correction_to_enthalpy(self): self.assertAlmostEqual(self.parser.thermal_correction_to_enthalpy(), THERMAL_CORRECTION_TO_ENTHALPY, places=2) - - def test_nwchem_material_is_a_molecule_without_being_told(self): - # Constructed WITHOUT is_non_periodic on purpose: rupy never passes it. - material = Material("material", self.parser, is_final_structure=True).serialize_and_validate() - self.assertTrue(material["isNonPeriodic"]) - self.assertEqual(material["lattice"]["type"], "CUB") - derived = {p["name"] for p in material["derivedProperties"]} - self.assertIn("inchi", derived) - self.assertIn("inchi_key", derived) - self.assertNotIn("volume", derived) - self.assertNotIn("density", derived) - - def test_nwchem_material_shares_one_cell_between_structures(self): - initial = Material("material", self.parser, is_initial_structure=True).serialize_and_validate() - final = Material("material", self.parser, is_final_structure=True).serialize_and_validate() - self.assertEqual(initial["lattice"], final["lattice"]) - - def test_nwchem_material_of_a_relaxed_molecule(self): - material = Material("material", self.parser, is_final_structure=True).serialize_and_validate() - self.assertAlmostEqual(material["lattice"]["a"], FINAL_CELL_EDGE_MULTISTEP, places=6) - self.assertDeepAlmostEqual( - [c["value"] for c in material["basis"]["coordinates"]], FINAL_CRYSTAL_COORDINATES_MULTISTEP, places=6 - ) diff --git a/tests/integration/properties/test_material.py b/tests/integration/properties/test_material.py index 087e7311..0844d91d 100644 --- a/tests/integration/properties/test_material.py +++ b/tests/integration/properties/test_material.py @@ -3,9 +3,11 @@ from typing import Dict, List from express.parsers.apps.espresso.parser import EspressoParser +from express.parsers.apps.nwchem.parser import NwchemParser from express.parsers.apps.vasp.parser import VaspParser from express.properties.material import Material from tests.fixtures.data import SI as data +from tests.fixtures.nwchem.references import FINAL_CELL_EDGE_MULTISTEP, FINAL_CRYSTAL_COORDINATES_MULTISTEP from tests.integration import IntegrationTestBase @@ -24,6 +26,10 @@ def vasp_parser(self): def espresso_parser(self): return EspressoParser(work_dir=self.workDir, stdout_file=self.stdoutFile) + @property + def nwchem_parser(self): + return NwchemParser(work_dir=self.workDir, stdout_file=self.stdoutFile) + @property def structure_string(self): manifest = self.getManifest() @@ -96,3 +102,26 @@ def test_material_from_structure(self): def test_material_serialize_and_validate(self): material = Material("material", self.vasp_parser, is_initial_structure=True, is_non_periodic=True) self.assertJsonEqual(material) + + def test_nwchem_material_is_a_molecule_without_being_told(self): + # Constructed WITHOUT is_non_periodic on purpose: rupy never passes it. + material = Material("material", self.nwchem_parser, is_final_structure=True).serialize_and_validate() + self.assertTrue(material["isNonPeriodic"]) + self.assertEqual(material["lattice"]["type"], "CUB") + derived = {p["name"] for p in material["derivedProperties"]} + self.assertIn("inchi", derived) + self.assertIn("inchi_key", derived) + self.assertNotIn("volume", derived) + self.assertNotIn("density", derived) + + def test_nwchem_material_shares_one_cell_between_structures(self): + initial = Material("material", self.nwchem_parser, is_initial_structure=True).serialize_and_validate() + final = Material("material", self.nwchem_parser, is_final_structure=True).serialize_and_validate() + self.assertEqual(initial["lattice"], final["lattice"]) + + def test_nwchem_material_of_a_relaxed_molecule(self): + material = Material("material", self.nwchem_parser, is_final_structure=True).serialize_and_validate() + self.assertAlmostEqual(material["lattice"]["a"], FINAL_CELL_EDGE_MULTISTEP, places=6) + self.assertDeepAlmostEqual( + [c["value"] for c in material["basis"]["coordinates"]], FINAL_CRYSTAL_COORDINATES_MULTISTEP, places=6 + ) diff --git a/tests/unit/properties/test_utils.py b/tests/unit/properties/test_utils.py new file mode 100644 index 00000000..80ba42ae --- /dev/null +++ b/tests/unit/properties/test_utils.py @@ -0,0 +1,58 @@ +import unittest + +from express.parsers.mixins.ionic import IonicDataMixin +from express.properties.material import Material +from express.properties.utils import box_molecule + + +def basis(coordinates): + return { + "units": "angstrom", + "elements": [{"id": idx, "value": "H"} for idx, _ in enumerate(coordinates)], + "coordinates": [{"id": idx, "value": value} for idx, value in enumerate(coordinates)], + } + + +COMPACT = basis([[0.0, 0.0, 0.2], [0.0, 0.0, -0.2]]) +EXPANDED = basis([[0.0, 0.0, 2.5], [0.0, 0.0, -2.5]]) + + +class RelaxationThatExpandsParser(IonicDataMixin): + is_non_periodic = True + + def initial_basis(self): + return COMPACT + + def final_basis(self): + return EXPANDED + + +class BoxMoleculeTest(unittest.TestCase): + """ + A relaxation that expands, which no committed fixture covers and which is the direction that + corrupts data: size the cell from the compact structure, centre the expanded one in it, and atoms + land outside the box, where they read as extra fragments and wreck the InChI. + """ + + def edge(self, selected_basis, parsed_bases): + return box_molecule(selected_basis, parsed_bases)[0]["vectors"]["a"][0] + + def test_cell_is_sized_to_the_largest_structure(self): + self.assertEqual(self.edge(COMPACT, [COMPACT, EXPANDED]), self.edge(EXPANDED, [EXPANDED])) + self.assertGreater(self.edge(EXPANDED, [EXPANDED]), self.edge(COMPACT, [COMPACT])) + + def test_every_structure_is_centered_inside_the_shared_cell(self): + for selected_basis in (COMPACT, EXPANDED): + lattice, centered = box_molecule(selected_basis, [COMPACT, EXPANDED]) + edge = lattice["vectors"]["a"][0] + for coordinate in centered["coordinates"]: + self.assertTrue(all(0.0 <= value <= edge for value in coordinate["value"])) + + def test_material_hands_over_every_parsed_basis(self): + parser = RelaxationThatExpandsParser() + lattices = [ + Material("material", parser, **{kwarg: True}).lattice + for kwarg in ("is_initial_structure", "is_final_structure") + ] + self.assertEqual(lattices[0], lattices[1]) + self.assertAlmostEqual(lattices[0]["a"], self.edge(EXPANDED, [EXPANDED]), places=6) From 9e8d4637ba895662dcab64f50dda6e82f037e3ba Mon Sep 17 00:00:00 2001 From: VsevolodX Date: Wed, 26 Aug 2026 10:11:28 -0700 Subject: [PATCH 13/16] SOF-8005: name the box_molecule arguments apart, say why there is no cell TB-NAME-1: `basis` and `bases` are one letter apart at a call site that passes both. Now selected_basis and parsed_bases. TB-DOC-1/3: NwchemParser's missing lattice methods are load-bearing control flow - material.py reads None as "molecule, derive a box" - and nothing said so once the old docstrings went with the methods. One sentence on the class, where a reader wondering why the methods are absent will be. Not repeated at the guard. Also from the review: basis() returns None with no geometry block, so say `dict | None` as the rest of the file does; box_molecule's docstring trimmed and its Returns made truthful; the padded cell gets a named intermediate instead of a bare [0][0]; and the fixture comment drops the SHA it cited, which will not survive a squash-merge. Co-Authored-By: Claude Fable 5 --- express/parsers/apps/nwchem/formats/txt.py | 2 +- express/parsers/apps/nwchem/parser.py | 3 +++ express/properties/utils.py | 30 ++++++++++------------ tests/fixtures/nwchem/references.py | 7 +++-- 4 files changed, 21 insertions(+), 21 deletions(-) diff --git a/express/parsers/apps/nwchem/formats/txt.py b/express/parsers/apps/nwchem/formats/txt.py index 253d60a1..a739fefe 100644 --- a/express/parsers/apps/nwchem/formats/txt.py +++ b/express/parsers/apps/nwchem/formats/txt.py @@ -52,7 +52,7 @@ def basis(self, text, index): index (int): position of the block among those printed. Returns: - dict + dict | None Example: { diff --git a/express/parsers/apps/nwchem/parser.py b/express/parsers/apps/nwchem/parser.py index 57f4185d..6f3e5511 100644 --- a/express/parsers/apps/nwchem/parser.py +++ b/express/parsers/apps/nwchem/parser.py @@ -11,6 +11,9 @@ class NwchemParser(BaseParser, IonicDataMixin, ElectronicDataMixin, ReciprocalDataMixin): """ Nwchem parser class. + + NWChem prints no cell for a molecule, so the lattice methods are left unimplemented and return + None: `express.properties.utils.box_molecule` derives the box, rather than a parser inventing it. """ is_non_periodic = True diff --git a/express/properties/utils.py b/express/properties/utils.py index 5d5bb7d3..afeb0565 100644 --- a/express/properties/utils.py +++ b/express/properties/utils.py @@ -33,30 +33,28 @@ def to_array_with_ids(array): return [{"id": index, "value": value} for index, value in enumerate(array)] -def box_molecule(basis, bases): +def box_molecule(selected_basis, parsed_bases): """ - Puts a molecule into the cell its application does not print: made's simple-cubic padding - convention, the same one that gives every non-periodic material on the platform its box. - - One cell for every basis in `bases`, so the structures of one calculation stay comparable: an - optimization moves atoms inside a fixed box rather than resizing it. Sized to whichever geometry - needs the most room, and the basis is then centered in it -- printed coordinates straddle the - origin, and atoms outside the box read as extra fragments and corrupt the InChI. + Derives the cell a molecule's application does not print -- made's simple-cubic padding, sized to + hold every structure of the calculation so an optimization moves atoms inside a fixed box rather + than resizing it -- and centers the selected basis in it. Printed coordinates straddle the origin, + and atoms outside the box read as extra fragments and corrupt the InChI. Args: - basis (dict): the basis to center. - bases (list): every basis the cell has to hold. + selected_basis (dict): the basis to center. + parsed_bases (list): every basis the cell has to hold. Returns: - tuple[dict, dict]: lattice vectors and the centered basis. + tuple[dict, dict]: lattice vectors, and the selected basis centered in them. """ - coordinates = [coordinate["value"] for coordinate in basis["coordinates"]] - edge = max( - calculate_padded_cell_simple_cubic([point["value"] for point in other["coordinates"]])[0][0] for other in bases - ) + coordinates = [coordinate["value"] for coordinate in selected_basis["coordinates"]] + cells = [ + calculate_padded_cell_simple_cubic([point["value"] for point in other["coordinates"]]) for other in parsed_bases + ] + edge = max(vectors[0][0] for vectors in cells) center = get_center_of_coordinates(coordinates) centered = [[x - center[axis] + edge / 2 for axis, x in enumerate(coordinate)] for coordinate in coordinates] return ( {"vectors": {"a": [edge, 0.0, 0.0], "b": [0.0, edge, 0.0], "c": [0.0, 0.0, edge], "alat": 1}}, - dict(basis, coordinates=to_array_with_ids(centered)), + dict(selected_basis, coordinates=to_array_with_ids(centered)), ) diff --git a/tests/fixtures/nwchem/references.py b/tests/fixtures/nwchem/references.py index a16b161c..1ece6559 100644 --- a/tests/fixtures/nwchem/references.py +++ b/tests/fixtures/nwchem/references.py @@ -40,8 +40,7 @@ } # test-001 declares `units au`, so these are its printed coordinates converted to angstrom. Read as -# angstrom instead they give an O-H of 1.81 A, which is what -# test_bond_length_of_an_atomic_units_block_is_physical guards against. +# angstrom instead they give an O-H of 1.81 A, which is what the bond length test guards against. BASIS = { "units": "angstrom", "elements": [{"id": 0, "value": "O"}, {"id": 1, "value": "H"}, {"id": 2, "value": "H"}], @@ -74,8 +73,8 @@ } # What express serializes for test-002's final block: made's cubic padding sized to fit both -# structures, and the basis centered in it. Taken from a488316, where the parser did that itself -- -# these are the numbers the move into the properties layer had to leave alone. +# structures, and the basis centered in it. These are the numbers the move of that work out of the +# parser and into the properties layer had to leave alone. FINAL_CELL_EDGE_MULTISTEP = 5.721712 FINAL_CRYSTAL_COORDINATES_MULTISTEP = [ [0.5, 0.5, 0.569589977], From 04f4d7d26bfb4001649b131ded6dcb7af0051277 Mon Sep 17 00:00:00 2001 From: VsevolodX Date: Wed, 26 Aug 2026 12:41:59 -0700 Subject: [PATCH 14/16] SOF-8005: revert formatter churn on files outside the ticket a488316 swept black reflows and blank-line fixes into seven files no nwchem change touches. Restore them to main byte-for-byte. Co-Authored-By: Claude Fable 5 --- tests/fixtures/data.py | 4 +- tests/fixtures/espresso/v7_2/references.py | 4 +- tests/fixtures/structural/references.py | 1 - .../non_scalar/test_dielectric_tensor.py | 41 ++++++++++++++----- .../test_wavefunction_amplitude.py | 8 +++- .../scalar/test_defect_formation_energy.py | 4 +- .../scalar/test_formation_energy.py | 2 +- 7 files changed, 44 insertions(+), 20 deletions(-) diff --git a/tests/fixtures/data.py b/tests/fixtures/data.py index 5e3cbd6f..c7c6f84f 100644 --- a/tests/fixtures/data.py +++ b/tests/fixtures/data.py @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:56094dbbcca726363c0128336d985b20141ea9e8462e4b130c3d19056ae7b0be -size 24845 +oid sha256:c4011c8acb2dfc0158add774a8a05646bef1612ee343c540d86f4e9ed5fcbdfe +size 24847 diff --git a/tests/fixtures/espresso/v7_2/references.py b/tests/fixtures/espresso/v7_2/references.py index 9d7147ec..f73efe71 100644 --- a/tests/fixtures/espresso/v7_2/references.py +++ b/tests/fixtures/espresso/v7_2/references.py @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:f1863b48f57554c2cb8154afd13cbaa824fbc53c287b42bfe05a5ad4aae7410c -size 6184 +oid sha256:4a7470e2d1313ced311dd737abf0e1e728b47aab58dc0bdf10d85b208c524fd4 +size 4964 diff --git a/tests/fixtures/structural/references.py b/tests/fixtures/structural/references.py index 553c5d30..6d26ed96 100644 --- a/tests/fixtures/structural/references.py +++ b/tests/fixtures/structural/references.py @@ -1,7 +1,6 @@ """ Reference values for the InChI test calculations within ExPrESS """ - INCHI_DATA = {"inchi": "1S/CH4/h1H4", "inchi_key": "VNWKTOKETHGBQD-UHFFFAOYSA-N"} # Reference data for Li CIF test (test-004) — verifies oxidation state stripping (Li0+ -> Li) diff --git a/tests/unit/properties/non_scalar/test_dielectric_tensor.py b/tests/unit/properties/non_scalar/test_dielectric_tensor.py index ce3576d2..5e5dcf7a 100644 --- a/tests/unit/properties/non_scalar/test_dielectric_tensor.py +++ b/tests/unit/properties/non_scalar/test_dielectric_tensor.py @@ -36,53 +36,72 @@ def test_dielectric_tensor(self): property_ = DielectricTensor("dielectric_tensor", parser) self.assertDeepAlmostEqual(property_.serialize_and_validate(), DIELECTRIC_TENSOR) - DIELECTRIC_TENSOR = { "name": "dielectric_tensor", "values": [ { "part": "real", "spin": 0.5, - "frequencies": [0.000000000, 0.060120240, 0.120240481, 0.180360721], + "frequencies": [ + 0.000000000, + 0.060120240, + 0.120240481, + 0.180360721 + ], "components": [ [20.137876673, 20.137876704, 20.137849785], [20.143821034, 20.143821066, 20.143794147], [20.161680126, 20.161680158, 20.161653237], [20.191532277, 20.191532311, 20.191505388], - ], + ] }, { "part": "imaginary", "spin": 0.5, - "frequencies": [0.000000000, 0.060120240, 0.120240481, 0.180360721], + "frequencies": [ + 0.000000000, + 0.060120240, + 0.120240481, + 0.180360721 + ], "components": [ [20.137876673, 20.137876704, 20.137849785], [20.143821034, 20.143821066, 20.143794147], [20.161680126, 20.161680158, 20.161653237], [20.191532277, 20.191532311, 20.191505388], - ], + ] }, { "part": "real", "spin": -0.5, - "frequencies": [0.000000000, 0.060120240, 0.120240481, 0.180360721], + "frequencies": [ + 0.000000000, + 0.060120240, + 0.120240481, + 0.180360721 + ], "components": [ [20.137876673, 20.137876704, 20.137849785], [20.143821034, 20.143821066, 20.143794147], [20.161680126, 20.161680158, 20.161653237], [20.191532277, 20.191532311, 20.191505388], - ], + ] }, { "part": "imaginary", "spin": -0.5, - "frequencies": [0.000000000, 0.060120240, 0.120240481, 0.180360721], + "frequencies": [ + 0.000000000, + 0.060120240, + 0.120240481, + 0.180360721 + ], "components": [ [20.137876673, 20.137876704, 20.137849785], [20.143821034, 20.143821066, 20.143794147], [20.161680126, 20.161680158, 20.161653237], [20.191532277, 20.191532311, 20.191505388], - ], - }, - ], + ] + } + ] } diff --git a/tests/unit/properties/non_scalar/two_dimensional_plot/test_wavefunction_amplitude.py b/tests/unit/properties/non_scalar/two_dimensional_plot/test_wavefunction_amplitude.py index f2535d19..a05b3293 100644 --- a/tests/unit/properties/non_scalar/two_dimensional_plot/test_wavefunction_amplitude.py +++ b/tests/unit/properties/non_scalar/two_dimensional_plot/test_wavefunction_amplitude.py @@ -3,12 +3,15 @@ RAW_DATA_ALAT = [ [0.0, 0.0050251256, 0.0100502513, 0.0150753769, 0.0201005025], - [0.0000322091, 0.0000072134, -0.0000218274, -0.0000540398, -0.0000883573], + [0.0000322091, 0.0000072134, -0.0000218274, -0.0000540398, -0.0000883573] ] ALAT_ANGSTROM = 10.0 -CONVERTED_DATA_ANGSTROMS = [[x * ALAT_ANGSTROM for x in RAW_DATA_ALAT[0]], RAW_DATA_ALAT[1]] +CONVERTED_DATA_ANGSTROMS = [ + [x * ALAT_ANGSTROM for x in RAW_DATA_ALAT[0]], + RAW_DATA_ALAT[1] +] EXPECTED = { "name": "wavefunction_amplitude", @@ -30,3 +33,4 @@ def test_wavefunction_amplitude(self): parser = self.get_mocked_parser("wavefunction_amplitude", CONVERTED_DATA_ANGSTROMS) property_ = WavefunctionAmplitude("wavefunction_amplitude", parser) self.assertDeepAlmostEqual(property_.serialize_and_validate(), EXPECTED) + diff --git a/tests/unit/properties/scalar/test_defect_formation_energy.py b/tests/unit/properties/scalar/test_defect_formation_energy.py index 94484159..8b7da432 100644 --- a/tests/unit/properties/scalar/test_defect_formation_energy.py +++ b/tests/unit/properties/scalar/test_defect_formation_energy.py @@ -12,5 +12,7 @@ def tearDown(self): super().tearDown() def test_defect_formation_energy(self): - property_ = ScalarPropertyFromContext("defect_formation_energy", None, value=DEFECT_FORMATION_ENERGY["value"]) + property_ = ScalarPropertyFromContext( + "defect_formation_energy", None, value=DEFECT_FORMATION_ENERGY["value"] + ) self.assertDeepAlmostEqual(property_.serialize_and_validate(), DEFECT_FORMATION_ENERGY) diff --git a/tests/unit/properties/scalar/test_formation_energy.py b/tests/unit/properties/scalar/test_formation_energy.py index fc7c1fa4..f35ef34d 100644 --- a/tests/unit/properties/scalar/test_formation_energy.py +++ b/tests/unit/properties/scalar/test_formation_energy.py @@ -12,6 +12,6 @@ def tearDown(self): super().tearDown() def test_formation_energy(self): - parser = self.get_mocked_parser("formation_energy", FORMATION_ENERGY["value"]) # noqa : F841 + parser = self.get_mocked_parser("formation_energy", FORMATION_ENERGY["value"]) # noqa : F841 property_ = ScalarPropertyFromContext("formation_energy", None, value=FORMATION_ENERGY["value"]) self.assertDeepAlmostEqual(property_.serialize_and_validate(), FORMATION_ENERGY) From 43bf26f837f5f8c10ee7ffcd5be21cffedaa7152 Mon Sep 17 00:00:00 2001 From: VsevolodX Date: Wed, 26 Aug 2026 13:29:12 -0700 Subject: [PATCH 15/16] SOF-8005: compress the diff, keeping docstrings and true-to-output fixtures Cuts the branch from 405 insertions over origin/main to 296. What went is prose and code; the data and the docstrings stayed. Code and prose: - the three Material tests merge into one, since all three read the same fixture, and the manifest loses two entries with them - test_converts_a_block_printed_in_atomic_units folds into the bond-length test, which asserts the same conversion more strictly - test_nwchem_structures_of_optimization goes, with its two basis constants. Its numbers are still pinned one layer up: the cell edge comes from test-002's initial block and the crystal coordinates from its final one, so the serialized-material test covers both - geometry_blocks() and box_molecule() keep the why-prose and lose the Args/Returns boilerplate; settings.py's regex comments keep their constraint and lose the restatement Data was not golfed. The unit fixtures print a real NWChem geometry block - header, column titles, dashed rule, column-aligned rows, atomic-mass tail - and the bond-length test carries test-001's three rows verbatim with both O-H distances asserted, because that is what the log contains. basis() keeps its Example, as its siblings have one, and the two parser delegates keep the full docstring the methods above them use. Coverage went up, not down: the truncation guard had no test at all and now has one. All four mutations still fail - centering, per-block sizing, the material.py guard, and the initial call site. The serialized material for test-001 and test-002 is byte-identical to 04f4d7d. Co-Authored-By: Claude Fable 5 --- express/parsers/apps/nwchem/formats/txt.py | 29 ++----- express/parsers/apps/nwchem/settings.py | 10 +-- express/properties/utils.py | 15 +--- tests/fixtures/nwchem/references.py | 29 +------ .../parsers/apps/nwchem/test_parser.py | 8 +- tests/integration/properties/test_material.py | 25 ++---- tests/manifest.yaml | 12 --- tests/unit/parsers/test_nwchem_txt_parser.py | 79 +++++++------------ tests/unit/properties/test_utils.py | 45 ++++------- 9 files changed, 73 insertions(+), 179 deletions(-) diff --git a/express/parsers/apps/nwchem/formats/txt.py b/express/parsers/apps/nwchem/formats/txt.py index a739fefe..65a7ad47 100644 --- a/express/parsers/apps/nwchem/formats/txt.py +++ b/express/parsers/apps/nwchem/formats/txt.py @@ -14,38 +14,25 @@ def __init__(self, work_dir): def geometry_blocks(self, text): """ - Extracts every "Output coordinates" block, or nothing at all if they disagree on which - atoms are present. A log truncated mid-table parses as a shorter molecule, and the callers - below cannot tell that from a real one -- rupy would publish the fragment as - `final_structure`, formula and InChI included. - - A block is printed in whichever units the input declared, and its header carries the factor - converting them to a.u., so both `units angstrom` and `units au` runs are read correctly. - - Args: - text (str): text to extract data from. - - Returns: - list[tuple[list[str], list[list[float]]]]: elements and coordinates in angstrom. + Extracts every "Output coordinates" block as elements and angstrom coordinates -- the header + carries the a.u. factor, so `units au` runs read correctly too -- or nothing at all if the + blocks disagree on their atoms: a log truncated mid-table parses as a shorter molecule, and + rupy would publish the fragment as `final_structure`. """ blocks = [] for block in settings.GEOMETRY_BLOCK_REGEX.finditer(text): to_angstrom = float(block.group("scale")) * Constant.BOHR rows = list(settings.GEOMETRY_ROW_REGEX.finditer(block.group("rows"))) - blocks.append( - ( - [row.group("element") for row in rows], - [[float(row.group(axis)) * to_angstrom for axis in ("x", "y", "z")] for row in rows], - ) - ) + elements = [row.group("element") for row in rows] + blocks.append((elements, [[float(row.group(axis)) * to_angstrom for axis in "xyz"] for row in rows])) if len({tuple(elements) for elements, _ in blocks}) > 1: return [] return blocks def basis(self, text, index): """ - Extracts the geometry block at the given index. An optimization prints one block per step; - a single-point run prints exactly one, so index 0 and index -1 then coincide. + Returns the geometry block at the given index. A single-point run prints exactly one block, + so index 0 and index -1 then coincide. Args: text (str): text to extract data from. diff --git a/express/parsers/apps/nwchem/settings.py b/express/parsers/apps/nwchem/settings.py index 541cbe96..0d130700 100644 --- a/express/parsers/apps/nwchem/settings.py +++ b/express/parsers/apps/nwchem/settings.py @@ -7,19 +7,15 @@ NWCHEM_OUTPUT_FILE_REGEX = "Northwest Computational Chemistry Package" GEOMETRY_HEADER_REGEX = r"Output coordinates in (?P\S+) \(scale by\s+(?P[\d.]+) to convert to a\.u\.\)" +# Requiring the tag to start with a symbol keeps a numeric-looking row of another table from matching. ELEMENT_REGEX = r"[A-Za-z]+" -# Requiring the tag to start with a symbol is what keeps a numeric-looking row of some other table -# from matching: the row shape alone is not specific enough. GEOMETRY_TAG_REGEX = r"{}\S*".format(ELEMENT_REGEX) GEOMETRY_RULE_REGEX = r"^[ \t]*-{4,}[- \t]*$\n" GEOMETRY_ROW_TEMPLATE = r"^[ \t]*\d+[ \t]+{tag}[ \t]+{double}[ \t]+{x}[ \t]+{y}[ \t]+{z}[ \t]*$" -# Anything up to the next block, and never across it, so a header is always paired with its own -# table rather than reaching forward into the following one. +# Never across the next block, so a header is always paired with its own table. UNTIL_NEXT_GEOMETRY_REGEX = r"(?:(?!Output coordinates in)[\s\S])*?" -# Geometry blocks are printed in whichever units the input declared; `scale` converts them to a.u. -# The trailing `(?:row)+` is what bounds the table: it stops at the first line that is not a row, -# which is the blank line after the last atom. +# The trailing `(?:row)+` bounds the table: it stops at the blank line after the last atom. GEOMETRY_BLOCK_REGEX = re.compile( GEOMETRY_HEADER_REGEX + UNTIL_NEXT_GEOMETRY_REGEX diff --git a/express/properties/utils.py b/express/properties/utils.py index afeb0565..4ecdda81 100644 --- a/express/properties/utils.py +++ b/express/properties/utils.py @@ -35,17 +35,10 @@ def to_array_with_ids(array): def box_molecule(selected_basis, parsed_bases): """ - Derives the cell a molecule's application does not print -- made's simple-cubic padding, sized to - hold every structure of the calculation so an optimization moves atoms inside a fixed box rather - than resizing it -- and centers the selected basis in it. Printed coordinates straddle the origin, - and atoms outside the box read as extra fragments and corrupt the InChI. - - Args: - selected_basis (dict): the basis to center. - parsed_bases (list): every basis the cell has to hold. - - Returns: - tuple[dict, dict]: lattice vectors, and the selected basis centered in them. + Returns lattice vectors and the selected basis centered in them: made's simple-cubic padding, + sized to hold every basis in `parsed_bases` so an optimization moves atoms inside a fixed box + rather than resizing it. Printed coordinates straddle the origin, and atoms left outside the box + read as extra fragments and corrupt the InChI. """ coordinates = [coordinate["value"] for coordinate in selected_basis["coordinates"]] cells = [ diff --git a/tests/fixtures/nwchem/references.py b/tests/fixtures/nwchem/references.py index 1ece6559..20f38c8c 100644 --- a/tests/fixtures/nwchem/references.py +++ b/tests/fixtures/nwchem/references.py @@ -39,8 +39,7 @@ "nuclear_repulsion": {"name": "nuclear_repulsion", "value": 250.20815670232923}, } -# test-001 declares `units au`, so these are its printed coordinates converted to angstrom. Read as -# angstrom instead they give an O-H of 1.81 A, which is what the bond length test guards against. +# test-001 declares `units au`, so these are its printed coordinates converted to angstrom. BASIS = { "units": "angstrom", "elements": [{"id": 0, "value": "O"}, {"id": 1, "value": "H"}, {"id": 2, "value": "H"}], @@ -51,30 +50,8 @@ ], } -# test-002 optimizes, so its first and last blocks differ. -# 6-31G* geometry: O-H 0.96866 A after relaxation, not the 6-31G 0.9758 A the Cypress feature pins. -INITIAL_BASIS_MULTISTEP = { - "units": "angstrom", - "elements": [{"id": 0, "value": "O"}, {"id": 1, "value": "H"}, {"id": 2, "value": "H"}], - "coordinates": [ - {"id": 0, "value": [0.0, 0.0, 0.22143053]}, - {"id": 1, "value": [-1.43042811, 0.0, -0.88572214]}, - {"id": 2, "value": [1.43042811, 0.0, -0.88572214]}, - ], -} -FINAL_BASIS_MULTISTEP = { - "units": "angstrom", - "elements": [{"id": 0, "value": "O"}, {"id": 1, "value": "H"}, {"id": 2, "value": "H"}], - "coordinates": [ - {"id": 0, "value": [0.0, 0.0, -0.11849741]}, - {"id": 1, "value": [-0.76261482, 0.0, -0.71575817]}, - {"id": 2, "value": [0.76261482, 0.0, -0.71575817]}, - ], -} - -# What express serializes for test-002's final block: made's cubic padding sized to fit both -# structures, and the basis centered in it. These are the numbers the move of that work out of the -# parser and into the properties layer had to leave alone. +# What express serializes for test-002: made's cubic padding sized to the larger of its two blocks, +# with the final basis centered in it. The edge therefore pins the initial block too. FINAL_CELL_EDGE_MULTISTEP = 5.721712 FINAL_CRYSTAL_COORDINATES_MULTISTEP = [ [0.5, 0.5, 0.569589977], diff --git a/tests/integration/parsers/apps/nwchem/test_parser.py b/tests/integration/parsers/apps/nwchem/test_parser.py index 777d5d8a..85c63c4f 100644 --- a/tests/integration/parsers/apps/nwchem/test_parser.py +++ b/tests/integration/parsers/apps/nwchem/test_parser.py @@ -51,10 +51,6 @@ def test_nwchem_structures_of_single_point(self): self.assertDeepAlmostEqual(self.parser.initial_basis(), BASIS, places=6) self.assertDeepAlmostEqual(self.parser.final_basis(), BASIS, places=6) - def test_nwchem_structures_of_optimization(self): - self.assertDeepAlmostEqual(self.parser.initial_basis(), INITIAL_BASIS_MULTISTEP, places=6) - self.assertDeepAlmostEqual(self.parser.final_basis(), FINAL_BASIS_MULTISTEP, places=6) - def test_nwchem_total_energy_contributions(self): self.assertDeepAlmostEqual(self.parser.total_energy_contributions(), TOTAL_ENERGY_CONTRIBUTION, places=2) @@ -65,4 +61,6 @@ def test_nwchem_thermal_correction_to_energy(self): self.assertAlmostEqual(self.parser.thermal_correction_to_energy(), THERMAL_CORRECTION_TO_ENERGY, places=2) def test_nwchem_thermal_correction_to_enthalpy(self): - self.assertAlmostEqual(self.parser.thermal_correction_to_enthalpy(), THERMAL_CORRECTION_TO_ENTHALPY, places=2) + self.assertAlmostEqual( + self.parser.thermal_correction_to_enthalpy(), THERMAL_CORRECTION_TO_ENTHALPY, places=2 + ) diff --git a/tests/integration/properties/test_material.py b/tests/integration/properties/test_material.py index 0844d91d..3512b20c 100644 --- a/tests/integration/properties/test_material.py +++ b/tests/integration/properties/test_material.py @@ -103,25 +103,14 @@ def test_material_serialize_and_validate(self): material = Material("material", self.vasp_parser, is_initial_structure=True, is_non_periodic=True) self.assertJsonEqual(material) - def test_nwchem_material_is_a_molecule_without_being_told(self): + def test_nwchem_material_of_a_relaxed_molecule(self): # Constructed WITHOUT is_non_periodic on purpose: rupy never passes it. - material = Material("material", self.nwchem_parser, is_final_structure=True).serialize_and_validate() - self.assertTrue(material["isNonPeriodic"]) - self.assertEqual(material["lattice"]["type"], "CUB") - derived = {p["name"] for p in material["derivedProperties"]} - self.assertIn("inchi", derived) - self.assertIn("inchi_key", derived) - self.assertNotIn("volume", derived) - self.assertNotIn("density", derived) - - def test_nwchem_material_shares_one_cell_between_structures(self): initial = Material("material", self.nwchem_parser, is_initial_structure=True).serialize_and_validate() - final = Material("material", self.nwchem_parser, is_final_structure=True).serialize_and_validate() - self.assertEqual(initial["lattice"], final["lattice"]) - - def test_nwchem_material_of_a_relaxed_molecule(self): material = Material("material", self.nwchem_parser, is_final_structure=True).serialize_and_validate() + coordinates = [c["value"] for c in material["basis"]["coordinates"]] + self.assertEqual(initial["lattice"], material["lattice"]) + self.assertEqual([material["isNonPeriodic"], material["lattice"]["type"]], [True, "CUB"]) self.assertAlmostEqual(material["lattice"]["a"], FINAL_CELL_EDGE_MULTISTEP, places=6) - self.assertDeepAlmostEqual( - [c["value"] for c in material["basis"]["coordinates"]], FINAL_CRYSTAL_COORDINATES_MULTISTEP, places=6 - ) + self.assertDeepAlmostEqual(coordinates, FINAL_CRYSTAL_COORDINATES_MULTISTEP, places=6) + derived = {p["name"] for p in material["derivedProperties"]} + self.assertEqual(derived & {"inchi", "inchi_key", "volume", "density"}, {"inchi", "inchi_key"}) diff --git a/tests/manifest.yaml b/tests/manifest.yaml index add5f83d..fad9041c 100644 --- a/tests/manifest.yaml +++ b/tests/manifest.yaml @@ -34,10 +34,6 @@ test_nwchem_structures_of_single_point: workDir: fixtures/nwchem/test-001 stdoutFile: fixtures/nwchem/test-001/nwchem-total-energy.log -test_nwchem_structures_of_optimization: - workDir: fixtures/nwchem/test-002 - stdoutFile: fixtures/nwchem/test-002/nwchem-frequency.log - test_nwchem_zero_point_energy: workDir: fixtures/nwchem/test-002 stdoutFile: fixtures/nwchem/test-002/nwchem-frequency.log @@ -265,14 +261,6 @@ test_espresso_hubbard_v_nn: workDir: fixtures/espresso/v7_2/test-009 stdoutFile: fixtures/espresso/v7_2/test-009/HUBBARD.dat -test_nwchem_material_is_a_molecule_without_being_told: - workDir: fixtures/nwchem/test-002 - stdoutFile: fixtures/nwchem/test-002/nwchem-frequency.log - -test_nwchem_material_shares_one_cell_between_structures: - workDir: fixtures/nwchem/test-002 - stdoutFile: fixtures/nwchem/test-002/nwchem-frequency.log - test_nwchem_material_of_a_relaxed_molecule: workDir: fixtures/nwchem/test-002 stdoutFile: fixtures/nwchem/test-002/nwchem-frequency.log diff --git a/tests/unit/parsers/test_nwchem_txt_parser.py b/tests/unit/parsers/test_nwchem_txt_parser.py index d3adbe57..a5af0a48 100644 --- a/tests/unit/parsers/test_nwchem_txt_parser.py +++ b/tests/unit/parsers/test_nwchem_txt_parser.py @@ -61,7 +61,7 @@ def test_returns_nothing_without_an_orbital_analysis_section(self): self.assertEqual(self.parser.eigenvalues_at_vectors(" Total DFT energy = -76.4\n"), []) -def geometry_block(units, scale, rows): +def geometry_block(units, scale, *rows): return "\n".join( [ f" Output coordinates in {units} (scale by {scale} to convert to a.u.)", @@ -77,72 +77,51 @@ def geometry_block(units, scale, rows): ) -class NwchemTXTParserGeometryTest(unittest.TestCase): - """ - Covers what the integration fixtures cannot: that the units the block declares are honoured, - and that a run with no geometry block at all returns None rather than raising. - """ +ANGSTROM = ("angstroms", "1.889725989") +ATOMIC_UNITS = ("a.u.", "1.000000000") +ANGSTROM_ROWS = [ + " 1 O 8.0000 0.00000000 0.00000000 1.00000000", + " 2 H 1.0000 0.00000000 0.00000000 -1.00000000", +] - ANGSTROM_ROWS = [ - " 1 O 8.0000 0.00000000 0.00000000 1.00000000", - " 2 H 1.0000 0.00000000 0.00000000 -1.00000000", - ] +class NwchemTXTParserGeometryTest(unittest.TestCase): def setUp(self): self.parser = NwchemTXTParser(work_dir=".") - def test_reads_first_and_last_block_of_an_optimization(self): - text = geometry_block("angstroms", "1.889725989", self.ANGSTROM_ROWS) + geometry_block( - "angstroms", - "1.889725989", - [ - " 1 O 8.0000 0.00000000 0.00000000 0.50000000", - " 2 H 1.0000 0.00000000 0.00000000 -0.50000000", - ], - ) - initial = self.parser.basis(text, 0)["coordinates"] - final = self.parser.basis(text, -1)["coordinates"] - self.assertAlmostEqual(initial[0]["value"][2] - initial[1]["value"][2], 2.0) - self.assertAlmostEqual(final[0]["value"][2] - final[1]["value"][2], 1.0) + def coordinates(self, text, index=-1): + return [[round(v, 6) for v in c["value"]] for c in self.parser.basis(text, index)["coordinates"]] - def test_converts_a_block_printed_in_atomic_units(self): - text = geometry_block( - "a.u.", - "1.000000000", - [ - " 1 O 8.0000 0.00000000 0.00000000 1.88972599", - " 2 H 1.0000 0.00000000 0.00000000 -1.88972599", - ], + def test_selects_the_block_by_index(self): + text = geometry_block(*ANGSTROM, *ANGSTROM_ROWS) + geometry_block( + *ANGSTROM, + " 1 O 8.0000 0.00000000 0.00000000 0.50000000", + " 2 H 1.0000 0.00000000 0.00000000 -0.50000000", ) - coordinates = self.parser.basis(text, -1)["coordinates"] - self.assertAlmostEqual(coordinates[0]["value"][2] - coordinates[1]["value"][2], 2.0, places=6) + self.assertEqual([self.coordinates(text, 0)[0][2], self.coordinates(text)[0][2]], [1.0, 0.5]) def test_bond_length_of_an_atomic_units_block_is_physical(self): - """test-001's rows verbatim. Read as angstrom they give an O-H of 1.81 A; the conversion is - what makes them the 0.9572 A of a real water molecule, and the BASIS fixture is those.""" + # test-001's rows verbatim. Read as angstrom they give an O-H of 1.81 A, not 0.9572 A. text = geometry_block( - "a.u.", - "1.000000000", - [ - " 1 O 8.0000 0.00000000 0.00000000 0.22143053", - " 2 H 1.0000 0.00000000 1.43042809 -0.88572213", - " 3 H 1.0000 0.00000000 -1.43042809 -0.88572213", - ], + *ATOMIC_UNITS, + " 1 O 8.0000 0.00000000 0.00000000 0.22143053", + " 2 H 1.0000 0.00000000 1.43042809 -0.88572213", + " 3 H 1.0000 0.00000000 -1.43042809 -0.88572213", ) - coordinates = [c["value"] for c in self.parser.basis(text, -1)["coordinates"]] + coordinates = self.coordinates(text) self.assertAlmostEqual(math.dist(coordinates[0], coordinates[1]), 0.9572, places=4) self.assertAlmostEqual(math.dist(coordinates[0], coordinates[2]), 0.9572, places=4) + def test_refuses_blocks_that_disagree_on_their_atoms(self): + # A log truncated mid-table parses as a shorter molecule, which rupy would publish. + text = geometry_block(*ANGSTROM, *ANGSTROM_ROWS) + geometry_block(*ANGSTROM, ANGSTROM_ROWS[0]) + self.assertIsNone(self.parser.basis(text, -1)) + def test_returns_nothing_without_a_geometry_block(self): self.assertIsNone(self.parser.basis(" Total DFT energy = -76.4\n", -1)) def test_ignores_a_numeric_row_that_happens_to_fit_the_shape(self): # A row of bare numbers satisfies the column shape but has no element symbol. It must be # skipped, not raise: rupy swallows the exception and final_structure vanishes silently. - text = geometry_block( - "angstroms", - "1.889725989", - self.ANGSTROM_ROWS + [" 3 1.0000 2.0000 3.0000 4.0000 5.0000"], - ) - basis = self.parser.basis(text, -1) - self.assertEqual([e["value"] for e in basis["elements"]], ["O", "H"]) + text = geometry_block(*ANGSTROM, *ANGSTROM_ROWS, " 3 1.0000 2.0000 3.0000 4.0000 5.0000") + self.assertEqual([e["value"] for e in self.parser.basis(text, -1)["elements"]], ["O", "H"]) diff --git a/tests/unit/properties/test_utils.py b/tests/unit/properties/test_utils.py index 80ba42ae..c757610a 100644 --- a/tests/unit/properties/test_utils.py +++ b/tests/unit/properties/test_utils.py @@ -2,22 +2,23 @@ from express.parsers.mixins.ionic import IonicDataMixin from express.properties.material import Material -from express.properties.utils import box_molecule +from express.properties.utils import box_molecule, to_array_with_ids -def basis(coordinates): +def basis(*heights): return { "units": "angstrom", - "elements": [{"id": idx, "value": "H"} for idx, _ in enumerate(coordinates)], - "coordinates": [{"id": idx, "value": value} for idx, value in enumerate(coordinates)], + "elements": to_array_with_ids(["H"] * len(heights)), + "coordinates": to_array_with_ids([[0.0, 0.0, height] for height in heights]), } -COMPACT = basis([[0.0, 0.0, 0.2], [0.0, 0.0, -0.2]]) -EXPANDED = basis([[0.0, 0.0, 2.5], [0.0, 0.0, -2.5]]) +COMPACT, EXPANDED = basis(0.2, -0.2), basis(2.5, -2.5) +EXPANDED_EDGE = 15.0 +STRUCTURE_KWARGS = ("is_initial_structure", "is_final_structure") -class RelaxationThatExpandsParser(IonicDataMixin): +class ExpandingRelaxationParser(IonicDataMixin): is_non_periodic = True def initial_basis(self): @@ -28,31 +29,17 @@ def final_basis(self): class BoxMoleculeTest(unittest.TestCase): - """ - A relaxation that expands, which no committed fixture covers and which is the direction that - corrupts data: size the cell from the compact structure, centre the expanded one in it, and atoms - land outside the box, where they read as extra fragments and wreck the InChI. - """ - - def edge(self, selected_basis, parsed_bases): - return box_molecule(selected_basis, parsed_bases)[0]["vectors"]["a"][0] - - def test_cell_is_sized_to_the_largest_structure(self): - self.assertEqual(self.edge(COMPACT, [COMPACT, EXPANDED]), self.edge(EXPANDED, [EXPANDED])) - self.assertGreater(self.edge(EXPANDED, [EXPANDED]), self.edge(COMPACT, [COMPACT])) - - def test_every_structure_is_centered_inside_the_shared_cell(self): + def test_shared_cell_holds_a_relaxation_that_expands(self): + # No committed fixture relaxes outward, and that is the direction that leaves atoms outside + # the box, where they read as extra fragments and corrupt the InChI. for selected_basis in (COMPACT, EXPANDED): lattice, centered = box_molecule(selected_basis, [COMPACT, EXPANDED]) - edge = lattice["vectors"]["a"][0] + self.assertEqual(lattice["vectors"]["a"][0], EXPANDED_EDGE) for coordinate in centered["coordinates"]: - self.assertTrue(all(0.0 <= value <= edge for value in coordinate["value"])) + self.assertTrue(all(0.0 <= x <= EXPANDED_EDGE for x in coordinate["value"])) def test_material_hands_over_every_parsed_basis(self): - parser = RelaxationThatExpandsParser() - lattices = [ - Material("material", parser, **{kwarg: True}).lattice - for kwarg in ("is_initial_structure", "is_final_structure") - ] + parser = ExpandingRelaxationParser() + lattices = [Material("material", parser, **{kwarg: True}).lattice for kwarg in STRUCTURE_KWARGS] self.assertEqual(lattices[0], lattices[1]) - self.assertAlmostEqual(lattices[0]["a"], self.edge(EXPANDED, [EXPANDED]), places=6) + self.assertAlmostEqual(lattices[0]["a"], EXPANDED_EDGE, places=6) From 73c339cd65a9c0d09109b98a4b11ea772425c162 Mon Sep 17 00:00:00 2001 From: "Timur (Tim) Bazhirov" Date: Wed, 26 Aug 2026 20:45:39 -0700 Subject: [PATCH 16/16] chore: update test_nwchem_txt_parser.py --- tests/unit/parsers/test_nwchem_txt_parser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/parsers/test_nwchem_txt_parser.py b/tests/unit/parsers/test_nwchem_txt_parser.py index a5af0a48..9c3d8c86 100644 --- a/tests/unit/parsers/test_nwchem_txt_parser.py +++ b/tests/unit/parsers/test_nwchem_txt_parser.py @@ -60,7 +60,7 @@ def test_reads_last_step_of_each_channel(self): def test_returns_nothing_without_an_orbital_analysis_section(self): self.assertEqual(self.parser.eigenvalues_at_vectors(" Total DFT energy = -76.4\n"), []) - +# NOTE: use fixtures in the future instead of inline strings below; tentatively, mat3ra-fixtures def geometry_block(units, scale, *rows): return "\n".join( [