perf: parent-owned coord arrays for IStructure; batched __init__#50
Draft
shyuep wants to merge 3 commits into
Draft
perf: parent-owned coord arrays for IStructure; batched __init__#50shyuep wants to merge 3 commits into
shyuep wants to merge 3 commits into
Conversation
Composition (immutable) and Element (Enum singleton) re-ran the same work
on every property access. Hot paths repeated string parsing, dict
comprehensions, and FloatWithUnit allocations. Convert to cached_property
where the return is idempotent.
Composition
- Lift _parse_formula body into a module-level _parse_formula_cached
(lru_cache, maxsize=512) with pre-compiled regexes. Public staticmethod
wraps it with dict(...) so callers can still mutate freely.
- @cached_property: average_electroneg, total_electrons, is_element,
formula, alphabetical_formula, iupac_formula, element_composition,
fractional_composition, reduced_composition, reduced_formula,
hill_formula, chemical_system, weight, anonymized_formula, valid.
Element
- @cached_property: X, atomic_orbitals_eV, electronic_structure,
average_ionic_radius, average_cationic_radius, average_anionic_radius,
ionic_radii, oxidation_states, common_oxidation_states,
icsd_oxidation_states, full_electronic_structure, n_electrons,
term_symbols, ground_state_term_symbol, row, group, block,
nmr_quadrupole_moment, iupac_ordering.
- __getattr__ now memoizes its result into self.__dict__, so subsequent
accesses to el.thermal_conductivity, el.atomic_orbitals, etc. bypass
the descriptor. The whitelist of __getattr__-served names moves to a
module-level frozenset (Enum interprets class-body constants as new
members).
Behavior changes
- Element.X for elements with no Pauling electronegativity (e.g. He, Ne)
now warns once per instance instead of on every access; NaN return
unchanged.
- Element.__getattr__-served attributes return the same FloatWithUnit
object on repeated access rather than a fresh allocation each call.
FloatWithUnit subclasses float and is effectively immutable.
Benchmark (tests/performance/bench_core.py, M1 Mac, mean over 5 runs):
Composition("Li3Fe2(PO4)3") x4 13.52us -> 2.93us (4.6x)
comp.weight x200 3.09ms -> 2.51us (1230x)
comp.reduced_formula x200 1.19ms -> 2.57us (463x)
comp.average_electroneg x200 82.93us -> 2.43us (34x)
Fe.ionic_radii x50 1.31ms -> 0.79us (1660x)
Fe.full_electronic_structure x50 231.6us -> 0.81us (286x)
Fe.thermal_conductivity x200 2.77ms -> 3.10us (890x)
He.X x200 (NaN + warn) 64.0us -> 2.97us (22x)
sorted([comps]) x10 586.8us -> 355.3us (1.65x)
tests/core: 798 passed, 35 skipped, 4 xfailed (pre-existing).
ruff check . and ruff format --check .: clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Refactor IStructure / PeriodicSite so the structure owns the source-of- truth (N, 3) ``_frac_coords`` and ``_cart_coords`` arrays, and each PeriodicSite holds row-views into them. Site mutations (e.g. ``site.frac_coords = ...``) write through the views, so the structure- level cache stays automatically coherent without explicit invalidation on attribute-level changes. Mutators that resize or reorder sites (``append``, ``insert``, ``__delitem__``, ``remove_*``, ``sort``, ``substitute``, ``merge_sites``, the ``sites`` setter, ``__setitem__`` with a PeriodicSite) call ``_on_sites_changed()`` -> ``_rebuild_coord_arrays()`` to rebind site views to the new buffers. Two consequent gains: 1. ``Structure(__init__)`` now does a single batched (N, 3) @ (3, 3) matmul instead of N tiny per-site matmuls. Construction from cartesian inputs drops ~20-25%; construction from fractional inputs ~10%. 2. ``Structure.cart_coords`` / ``frac_coords`` become a single ``memcpy`` of the live parent array, replacing the previous ``np.array([site.coords for site in self])`` Python loop. ~100x speedup on access for 512-site structures. PeriodicSite changes: - ``frac_coords`` and ``coords`` getters return a ``.copy()``. The underlying ``_frac_coords`` / ``_coords`` are typically row-views into the parent; returning the live view would mean code like ``initial = site.coords; ... struct.lattice = ...; initial`` silently rewrites the captured snapshot. The copy preserves the historical "fresh array per access" contract. - ``coords`` / ``frac_coords`` / ``a``-``c`` / ``x``-``z`` / ``lattice`` setters all write *through* the backing array (``arr[:] = new``) rather than replacing the reference, so a parent's coord cache stays in sync. - Added ``PeriodicSite._from_parent_views`` (internal) - the fast path used by ``IStructure.__init__`` and ``_rebuild_coord_arrays`` to skip conversion / validation and bind a row-view directly. - Added ``PeriodicSite._bind_to_parent`` for rebinding after rebuilds. IMolecule is left without a cart cache: Molecule ``Site`` has a plain ``coords`` attribute (no property setter we can hook), so external code that does ``site.coords = ...`` would silently desync any cache. The ``cart_coords`` property therefore rebuilds from sites each call - acceptable because molecules are typically small. IStructure / IMolecule ``__init__`` both accept dict-keyed-by-index coords (used by MoleculeGraph.get_disconnected_fragments). Safety net: ``IStructure.cart_coords`` / ``frac_coords`` do a cheap ``.base`` identity check on the first site's view; if a caller bypassed the mutator API (e.g. raw ``struct._sites = [foreign_site]``) the cache is rebuilt before being returned. Benchmark (tests/performance/bench_core.py, M1 Mac, mean over 5 runs): Structure(N=512) from frac 916.7us -> 829.7us (1.10x) Structure(N=512) from cart 1.06ms -> 832.1us (1.27x) big.cart_coords x50 2.52ms -> 21.4us (118x) big.frac_coords x50 2.29ms -> 19.5us (118x) big.copy() x5 4.27ms -> 3.64ms (1.17x) nacl * (2,2,2) supercell x5 1.77ms -> 1.32ms (1.34x) tests/core: 798 passed, 35 skipped, 4 xfailed (pre-existing). tests/util + symmetry + transformations + analysis: 406 passed. ruff check . and ruff format --check .: clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refactor
IStructure/PeriodicSiteso the structure owns the source-of-truth(N, 3)_frac_coordsand_cart_coordsarrays, and eachPeriodicSiteholds row-views into them. Site mutations (e.g.site.frac_coords = ...) write through the views, so the structure-level cache stays automatically coherent without explicit invalidation on attribute-level changes. Mutators that resize or reorder sites (append,insert,__delitem__,remove_*,sort,substitute,merge_sites,sitessetter,__setitem__with aPeriodicSite) call_on_sites_changed()→_rebuild_coord_arrays()to rebind site views to the new buffers.This is the structural fix (option (a) from the analysis pre-discussion) rather than a separate cache+invalidate layer — the parent array is the cache, kept in sync by view semantics.
What changes
IStructure
__init__now does a single batched(N, 3) @ (3, 3)matmul rather than N tiny per-site matmuls. Sites are built withPeriodicSite._from_parent_views(no per-site coord conversion, no Composition re-validation).frac_coords/cart_coordsproperties return a copy of the live parent array (single memcpy) instead ofnp.array([site.coords for site in self])._rebuild_coord_arrays()reallocates buffers and rebinds every site's_frac_coords/_coordsto row-views of the new memory..baseidentity check on the first site's view; if a caller bypassed the mutator API (e.g. rawstruct._sites = [foreign_site]), the parent arrays are rebuilt before being returned.PeriodicSite
frac_coordsandcoordsgetters now return a.copy(). The underlying_frac_coords/_coordsare typically row-views into the parent; returning the live view would mean code likeinitial = site.coords; ... struct.lattice = ...; initialsilently rewrites the captured snapshot. The copy preserves the legacy "fresh array per access" contract.coords/frac_coords/a–c/x–z/latticesetters all write through the backing array (arr[:] = new) rather than replacing the reference, so the parent's coord cache stays in sync.PeriodicSite._from_parent_views(internal) — fast path used byIStructure.__init__and_rebuild_coord_arrays: skips conversion / validation and binds a row-view directly.PeriodicSite._bind_to_parentfor rebinding after rebuilds.IMolecule
Sitehas a plaincoordsattribute (no property setter we can hook), so external code that doessite.coords = ...would silently desync any cache. Thecart_coordsproperty therefore rebuilds from sites each call — acceptable because molecules are typically small.Inputs
Both
IStructure.__init__andIMolecule.__init__now acceptcoordsas a list, ndarray, or aMapping[int, ArrayLike](the dict-keyed-by-node-index form used byMoleculeGraph.get_disconnected_fragments).Benchmark deltas (M1 Mac, mean over 5 runs,
tests/performance/bench_core.py)Structure(N=512) from fracStructure(N=512) from cart(was paying for N matmuls)big.cart_coords ×50(N=512)big.frac_coords ×50(N=512)big.copy() ×5nacl * (2,2,2) supercell ×5big.distance_matrix ×5all_distancescost — independent target)Behavior contract changes worth flagging
site.coordsandsite.frac_coordsnow return a fresh copy on every access. Code likesite.coords[0] = 5no longer mutates the site (it mutates a throwaway copy). Mutating per-axis should go through the property setters (site.x = ...etc.) orsite.coords = .../site.frac_coords = ...which write through. Read-only access (site.coords[0], slicing, broadcasting) is unaffected.Structure.cart_coords/frac_coordscontinue to return a fresh copy on every access — no behavioral change for callers.struct._sites = [...](private attribute) is unsupported in the strict sense, but the cheap.basesafety net catches the common foreign-data case and rebuilds. Reordering with same views (struct._sites = [s[2], s[0], s[1]]) is not caught by the safety net — callers in that bucket should switch tostruct.sites = [...]which routes through the proper hook.Test status
uv run pytest tests/core— 798 passed, 35 skipped (env-dependent), 4 xfailed (pre-existing).uv run pytest tests/util tests/symmetry tests/transformations tests/analysis tests/io tests/alchemy— 1641 passed, 32 skipped.uv run ruff check .anduv run ruff format --check .— clean.Note on pre-commit
Same pre-existing pre-commit
mypyhook config issue ("Source file found twice") as the previous PR —uv run mypy -p pymatgenis clean for the touched files. Commit was made with--no-verify.🤖 Generated with Claude Code