Skip to content

fix: raise KeyError (not ValueError) for missing custom property#12

Open
speckhard wants to merge 4 commits intoLeMaterial:mainfrom
speckhard:fix/keyerror-consistency
Open

fix: raise KeyError (not ValueError) for missing custom property#12
speckhard wants to merge 4 commits intoLeMaterial:mainfrom
speckhard:fix/keyerror-consistency

Conversation

@speckhard
Copy link
Copy Markdown

@speckhard speckhard commented Apr 28, 2026

Summary

Make the two indexing patterns for missing custom properties agree on the exception type:

call before after
mol["missing"] KeyError KeyError
mol.get_property("missing") ValueError KeyError

Both now raise KeyError with the same message (Property 'X' not found).

Why this is a behavior change worth shipping

  • KeyError is the Python-native exception type for "key not found in a mapping-like object", and it's already what mol[key] raised. Code that did try: ... except KeyError: worked for mol[key] but silently failed for mol.get_property(key) because the latter slipped through as ValueError.
  • The discrepancy was actively locked in by tests (one in test_atom_molecule.py, one in test_database.py) — those are updated.
  • The exception message string is unchanged, so anything matching on the message text still works.

Breaking-change scope

Callers who specifically caught ValueError only for get_property will need to switch to KeyError. There is no in-repo caller doing that; the only places asserting ValueError were the two tests this PR updates.

If broader impact is a concern, this can land in 0.3.0 instead of a 0.2.x patch — happy to defer.

Files

  • atompack-py/src/molecule.rs — two PyValueError::new_err(...)PyKeyError::new_err(...). PyKeyError was already imported in src/lib.rs.
  • atompack-py/python/atompack/__init__.pyiRaises section in the user-facing get_property docstring.
  • atompack-py/python/atompack/_atompack_rs.pyiRaises section also added to the internal stub for consistency.
  • atompack-py/tests/test_atom_molecule.py — flip 2 assertions; new test_missing_property_raises_keyerror_consistently locks in the symmetry so future drift fails loudly.
  • atompack-py/tests/test_database.py — flip 1 assertion.

Test plan

  • Targeted tests (touched files) green after maturin develop --release:
$ uv run --extra dev --locked pytest tests/test_atom_molecule.py tests/test_database.py -v
============================== 42 passed in 1.11s ==============================
tests/test_atom_molecule.py::test_molecule_custom_properties PASSED
tests/test_atom_molecule.py::test_missing_property_raises_keyerror_consistently PASSED
tests/test_database.py::test_database_single_item_mutation_is_copy_on_write[none-False] PASSED
... (all 42)
  • Full suite green (no regression elsewhere):
$ uv run --extra dev --locked pytest tests/ -q
124 passed, 6 skipped in 2.27s

(124 = previous 123 + the new test_missing_property_raises_keyerror_consistently.)

  • Interactive sanity check:
>>> import atompack, numpy as np
>>> mol = atompack.Molecule.from_arrays(
...     np.zeros((1, 3), np.float32), np.ones((1,), np.uint8))
>>> mol.get_property("missing")
Traceback (most recent call last):
  ...
KeyError: "Property 'missing' not found"
>>> mol["missing"]
Traceback (most recent call last):
  ...
KeyError: "Property 'missing' not found"

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.
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.
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.
@speckhard speckhard marked this pull request as ready for review April 30, 2026 15:17
@speckhard speckhard changed the title [DRAFT] fix: raise KeyError (not ValueError) for missing custom property fix: raise KeyError (not ValueError) for missing custom property Apr 30, 2026
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 LeMaterial#12.
speckhard added a commit to speckhard/atompack that referenced this pull request Apr 30, 2026
The new test_auto_max_size_caps_at_default_max test had two assert_eq!
calls that exceeded max_width=100 once DEFAULT_MAX_DECOMPRESSED_SIZE
was inlined. rustfmt's canonical form splits them across multiple lines.
Caught proactively by mirroring the cargo fmt --all --check pass that
PR LeMaterial#12 needed for its CI.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant