From 49ca267029a3a67f4e898cb1ac4dd23e8ace9007 Mon Sep 17 00:00:00 2001 From: dts Date: Tue, 28 Apr 2026 23:40:02 +0200 Subject: [PATCH 1/4] fix: raise KeyError (not ValueError) for missing custom property mol.get_property("missing") now raises KeyError, matching mol["missing"] which already raises KeyError. The two access patterns previously disagreed on the exception type for the same condition. Behavior change: callers catching ValueError specifically for get_property will need to switch to KeyError. The exception message and its str() are unchanged. Two in-repo tests that asserted ValueError have been updated. --- atompack-py/python/atompack/__init__.pyi | 2 +- atompack-py/src/molecule.rs | 4 ++-- atompack-py/tests/test_atom_molecule.py | 4 ++-- atompack-py/tests/test_database.py | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/atompack-py/python/atompack/__init__.pyi b/atompack-py/python/atompack/__init__.pyi index 880939c..938f4df 100644 --- a/atompack-py/python/atompack/__init__.pyi +++ b/atompack-py/python/atompack/__init__.pyi @@ -327,7 +327,7 @@ class Molecule: Raises ------ - ValueError + KeyError If property key does not exist """ ... diff --git a/atompack-py/src/molecule.rs b/atompack-py/src/molecule.rs index f881e2f..b8db6ed 100644 --- a/atompack-py/src/molecule.rs +++ b/atompack-py/src/molecule.rs @@ -667,7 +667,7 @@ impl PyMolecule { if let Some(inner) = molecule.as_owned() { return match inner.properties.get(key) { Some(v) => property_value_to_pyobject(py, v), - None => Err(PyValueError::new_err(format!( + None => Err(PyKeyError::new_err(format!( "Property '{}' not found", key ))), @@ -678,7 +678,7 @@ impl PyMolecule { })?; match view.find_custom_section(KIND_MOL_PROP, key)? { Some(section) => property_section_to_pyobject(py, view, section), - None => Err(PyValueError::new_err(format!( + None => Err(PyKeyError::new_err(format!( "Property '{}' not found", key ))), diff --git a/atompack-py/tests/test_atom_molecule.py b/atompack-py/tests/test_atom_molecule.py index 0ff034c..24195c6 100644 --- a/atompack-py/tests/test_atom_molecule.py +++ b/atompack-py/tests/test_atom_molecule.py @@ -225,10 +225,10 @@ def test_molecule_custom_properties() -> None: "vec3_f64", } - with pytest.raises(ValueError, match=r"not found"): + with pytest.raises(KeyError, match=r"not found"): mol.get_property("stress") - with pytest.raises(ValueError, match=r"not found"): + with pytest.raises(KeyError, match=r"not found"): mol.get_property("does_not_exist") diff --git a/atompack-py/tests/test_database.py b/atompack-py/tests/test_database.py index 7fbb075..52c9946 100644 --- a/atompack-py/tests/test_database.py +++ b/atompack-py/tests/test_database.py @@ -387,7 +387,7 @@ def test_database_single_item_mutation_is_copy_on_write( assert fresh.energy == pytest.approx(-5.0) np.testing.assert_allclose(fresh.forces, original.forces) assert fresh.get_property("tag") == "train" - with pytest.raises(ValueError, match="Property 'new_ids' not found"): + with pytest.raises(KeyError, match="Property 'new_ids' not found"): fresh.get_property("new_ids") From 086c90934ed30c7d4eb72516bbe83480918d78dd Mon Sep 17 00:00:00 2001 From: dts Date: Wed, 29 Apr 2026 10:27:48 +0200 Subject: [PATCH 2/4] test: lock in KeyError consistency between indexing patterns Independent review caught two follow-ups: - _atompack_rs.pyi PyMolecule.get_property stub now documents the KeyError it now raises. - New test asserts mol["x"] and mol.get_property("x") raise the same exception type so the previous drift can't recur silently. --- atompack-py/python/atompack/_atompack_rs.pyi | 5 +++++ atompack-py/tests/test_atom_molecule.py | 12 ++++++++++++ 2 files changed, 17 insertions(+) diff --git a/atompack-py/python/atompack/_atompack_rs.pyi b/atompack-py/python/atompack/_atompack_rs.pyi index 5235b99..99e8730 100644 --- a/atompack-py/python/atompack/_atompack_rs.pyi +++ b/atompack-py/python/atompack/_atompack_rs.pyi @@ -321,6 +321,11 @@ class PyMolecule: ------- Any Property value + + Raises + ------ + KeyError + If property key does not exist """ ... diff --git a/atompack-py/tests/test_atom_molecule.py b/atompack-py/tests/test_atom_molecule.py index 24195c6..50d4caf 100644 --- a/atompack-py/tests/test_atom_molecule.py +++ b/atompack-py/tests/test_atom_molecule.py @@ -232,6 +232,18 @@ def test_molecule_custom_properties() -> None: mol.get_property("does_not_exist") +def test_missing_property_raises_keyerror_consistently() -> None: + # Lock in symmetry: both indexing patterns must raise the same exception + # type for missing custom properties. They previously disagreed. + mol = _make_molecule() + mol.set_property("method", "DFT") + + with pytest.raises(KeyError, match=r"not found"): + _ = mol["does_not_exist"] + with pytest.raises(KeyError, match=r"not found"): + mol.get_property("does_not_exist") + + def test_molecule_getitem_supports_int_and_str() -> None: mol = _make_molecule() mol.set_property("method", "DFT") From c6b85ae1eff729321950d5009a79536b1610b7e5 Mon Sep 17 00:00:00 2001 From: dts Date: Thu, 30 Apr 2026 16:02:14 +0200 Subject: [PATCH 3/4] test: also flip the two test_from_ase ValueError assertions to KeyError The first commit on this branch updated test_atom_molecule.py and test_database.py but missed two analogous assertions in test_from_ase.py that exercise mol.get_property("stress") and mol.get_property("forces") on absent keys. With the KeyError change to molecule.rs::get_property, these tests would have failed at merge time. --- atompack-py/tests/test_from_ase.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/atompack-py/tests/test_from_ase.py b/atompack-py/tests/test_from_ase.py index 542cdd4..705a405 100644 --- a/atompack-py/tests/test_from_ase.py +++ b/atompack-py/tests/test_from_ase.py @@ -130,7 +130,7 @@ def test_from_ase_extracts_core_fields() -> None: mol.stress, np.array([[1.1, 1.2, 1.3], [2.1, 2.2, 2.3], [3.1, 3.2, 3.3]], dtype=np.float64), ) - with pytest.raises(ValueError, match=r"not found"): + with pytest.raises(KeyError, match=r"not found"): mol.get_property("stress") assert mol.has_property("unsupported") is False @@ -188,7 +188,7 @@ def test_from_ase_extracts_arrays_and_calc_results() -> None: mol.get_property("magmoms"), np.array([0.3, 0.4], dtype=np.float64) ) np.testing.assert_allclose(mol.stress, np.eye(3, dtype=np.float64) * 3.0) - with pytest.raises(ValueError, match=r"not found"): + with pytest.raises(KeyError, match=r"not found"): mol.get_property("forces") From 4d8d7e38967c4e4c9a0fc5facbff774fe8f1c6ae Mon Sep 17 00:00:00 2001 From: dts Date: Thu, 30 Apr 2026 17:58:33 +0200 Subject: [PATCH 4/4] style: cargo fmt for shorter PyKeyError lines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The earlier ValueError → PyKeyError flip kept the original multi-line format. PyKeyError is shorter than PyValueError, so the format!() call now fits on one line under max_width=100, and rustfmt's canonical form demands the single-line variant. Caught by ci-rust (rust-fmt-check) on PR #12. --- atompack-py/src/molecule.rs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/atompack-py/src/molecule.rs b/atompack-py/src/molecule.rs index b8db6ed..78a83e9 100644 --- a/atompack-py/src/molecule.rs +++ b/atompack-py/src/molecule.rs @@ -667,10 +667,7 @@ impl PyMolecule { if let Some(inner) = molecule.as_owned() { return match inner.properties.get(key) { Some(v) => property_value_to_pyobject(py, v), - None => Err(PyKeyError::new_err(format!( - "Property '{}' not found", - key - ))), + None => Err(PyKeyError::new_err(format!("Property '{}' not found", key))), }; } let view = molecule.as_view().ok_or_else(|| { @@ -678,10 +675,7 @@ impl PyMolecule { })?; match view.find_custom_section(KIND_MOL_PROP, key)? { Some(section) => property_section_to_pyobject(py, view, section), - None => Err(PyKeyError::new_err(format!( - "Property '{}' not found", - key - ))), + None => Err(PyKeyError::new_err(format!("Property '{}' not found", key))), } }