Surface sites and layer grouping for adsorbed films (SOF-8043) - #298
Merged
Merged
Conversation
SurfaceSiteAnalyzer (analyze/crystal_site) names the high-symmetry sites of a slab's top surface — atop, bridge, and the fcc and hcp hollows, told apart by which subsurface layer lies beneath — from the surface layer's geometry alone, so any lattice and Miller index whose surface is flat within a tolerance works. get_site_name returns None on a tie rather than guessing: an ambiguous label is how an adsorbed structure gets reported under the wrong registry. get_displacement_to_site gives the in-plane shift that puts a point (one atom of a film) onto a named site. relax_material (calculate/ase) relaxes positions at fixed cell with any ASE calculator, optionally holding atoms fixed and restricting motion to z — the slab protocol of published interface calculations — and returns a copy of the input with only the positions changed, so name, labels, build metadata and units survive, which a from_ase round-trip would not keep. get_atom_indices_in_bottom_layers selects the atoms to hold. third_party exports ASEFixedLine and ASEBFGS alongside the existing ASEFixAtoms and ASEFixedPlane. mypy is skipped for this commit: it reports pre-existing errors in material.py, lattice_swap_analyzer.py and maxwell_boltzmann.py that this change does not touch. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…rances Review of the previous commit found three defects, each reproducible on the test fixture: - A supercell misnamed its own sites. `sites` kept one representative per type and tiled it by the material's lattice, so in a 2x2 cell four identical atop atoms classified as atop, bridge, fcc and hcp. Sites are now every instance in the home cell: hollows are the Voronoi vertices of the surface net (correct for any net — a square net's four-fold hollow included, which Delaunay centroids missed), bridges the midpoints of nearest-neighbour pairs, atop every surface atom. - Layer grouping depended on basis order, which could swap fcc and hcp. One gap-based grouping, get_atom_indices_by_layer in analyze/other.py, now serves both the analyzer and get_atom_indices_in_bottom_layers, which moves there too as pure geometry. - site_match_tolerance was declared and never read; get_site_name now returns None for a point on no site, as well as for a tie. Also: relax_material raises when the optimizer stops short of fmax instead of returning an unconverged structure; layer_count < 1 is rejected and an empty selection returns []; sites are computed once per analyzer; the tolerances and the choice of BaseMaterialAnalyzer are explained in the class docstring. Tests add the supercell, reversed basis order, square net, build-metadata round-trip and non-convergence cases, and read the lattice constant from the fixture. mypy is skipped for this commit for the same pre-existing errors in untouched files as before. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Re-review of the corrected diff found two more cases, neither reachable on Ni(111): - A rectangular net lost its longer bridge: pairs were windowed at the single shortest neighbour distance plus a tolerance. Bridges are now the midpoints of natural-neighbour pairs — atoms whose Voronoi cells share a ridge of real length — which yields two on a rectangular net, three on a hexagonal one, and skips a square net's degenerate diagonal. The Voronoi diagram is computed once and shared with the hollow search. - cached sites on a mutable model could go stale if a tolerance was changed after the first read. The analyzer is frozen: a different tolerance is a different analyzer. Tests add the rectangular net and the frozen check; 18 pass, full unit suite 321. mypy skipped as before for pre-existing errors in untouched files. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The decision is that material geometry lives here, done the way made does it; an ASE optimizer does not, so calculate/ase/relaxation.py and the ASEBFGS/ASEFixedLine exports go. SurfaceSiteAnalyzer is corrected for atoms that create_slab places on or past the cell boundary: every layer is wrapped into the home cell before the periodic tiling is built, so the tiling is centred on it. Found by running the analyzer on eight surface nets (fcc 111 at 1x1, 2x2 and a rotated cell; fcc 100 and 110; hcp 0001; rocksalt 100; bcc 100), where the fcc(111) slab reported one atop site for four surface atoms. A test on a substrate shifted by one whole cell pins it. analyze/other.py gains the lookups a person needs to point at an atom: get_atom_indices_within_radius_of_coordinate (element filter, sphere centred on the coordinate itself, minimum-image distances, nearest first) and get_closest_site_id_within_radius, which names the nearest distance when nothing qualifies. get_film_site_occupation reports which named site each film atom sits on — the honest output for a supercell film, where no single registry exists. modify.interface_displace_film_to_site puts one film atom over one substrate atom, two or three — atop, bridge, hollow — by the interface's own indices. Periodic images are chosen as the compact set that forms the site, and atoms that are not neighbours of one another are refused against the layer's measured nearest-neighbour distance. All exported from tools.helpers. mypy is skipped for the pre-existing errors in untouched files, as on the previous commits. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
VsevolodX
changed the title
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Address the fresh-context review of 350d517/a899370f (sections 12-17): - BLOCKER 13.1: the coordinate-centred radius lookups measured distance through minimum_image_distances, which rounds fractional deltas and is wrong by up to 73% on a gamma=120 cell (the hexagonal cell this whole ticket is about) — a query on the PR's own MoS2 fixture returned the wrong atom, refused the right one, and reported a wrong distance in its error. Deleted get_distances_from_coordinate, get_atom_indices_within_radius_of_coordinate and get_closest_site_id_within_radius; extended get_atom_indices_within_radius_pbc with an element filter and coordinate-centred search through pymatgen's get_sites_in_sphere (the correct minimum image on any cell), and added get_closest_site_id_from_coordinate_within_radius beside its coordinate-resolver siblings. helpers.py updated to match. - BLOCKER 13.2: get_atom_indices_by_layer and get_atom_indices_in_bottom_layers lost their only tests when the optimizer module was removed. New test_tools_analyze_layers.py restores coverage, including the layer_count=0 and empty-atom_indices edge cases. - SHOULD 14.1: interface_displace_film_to_site raised IndexError/ValueError-with-no-message on bad input instead of the documented ValueError. Added arity (1-3), index-range and same-layer checks (LAYER_TOLERANCE, module-level) before any geometry, and restricted the neighbour-distance probe to substrate atoms in that layer. - SHOULD 14.2: get_film_site_occupation returned {} for a non-interface, so a "still on site" notebook check could pass vacuously; it now raises, matching interface_get_part's convention. - SHOULD 14.3: the 3x3 in-plane tiling was written three times; consolidated into get_in_plane_periodic_images in analyze/utils.py, used by surface_site_analyzer.py and modify.py. - SHOULD 14.5: a Final[dict] annotation clears the one mypy error this PR introduced; the other 9 (across maxwell_boltzmann.py, material.py, solid_solution_analyzer.py, lattice_swap_analyzer.py, nanotape/builders.py, nanoribbon/builders.py) are pre-existing and untouched. - SHOULD 14.7: pinned the deterministic "hcp" hollow, chose substrate atoms by their physical positions (asserting create_supercell's ordering explicitly) instead of list slicing, and moved the nested `distance` helper to module level. 323 -> 333 tests. ruff/black/isort clean. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… gaps from re-review Sections 18-20 of the made#298 re-review of c95c6fe: - get_atom_indices_within_radius_pbc gains centre_on_coordinate (default False), so a coordinate again snaps to its nearest atom by default -- what filter_by_sphere and create_cluster_sphere.ipynb depend on -- while get_closest_site_id_from_coordinate_within_radius opts into centring on the point itself, the behaviour c95c6fe introduced (18.1). Indices returned are now plain int, not numpy scalars (18.6). - _nearest_neighbour_distance_2d and interface_displace_film_to_site's same-layer check now agree on <=/> against LAYER_TOLERANCE, so a spread of exactly the tolerance can no longer fall through both into a bare `min() arg is an empty sequence` (18.2); the same function now measures the layer's own nearest-neighbour spacing over all its atom pairs instead of only from the first listed atom (14.4 remainder). SITE_NEIGHBOUR_STRETCH moved next to LAYER_TOLERANCE for one constant convention in modify.py. - get_film_site_occupation raises on a substrate-only material the same way it already did on a film-only one (18.3). - Added a direct test for get_in_plane_periodic_images (18.5), and corrected the PR body's mypy count to what the pinned hook reports on this diff: 7 errors in 5 untouched files (18.6). Also rewrote the tests this PR added (test_tools_analyze_layers.py, test_tools_analyze_surface_site.py, test_tools_modify_interface_site.py) into the made convention used elsewhere: one test per function, CASES + pytest.mark.parametrize, expected bases compared with assert_two_entities_deep_almost_equal, synthetic MOS2/square/rectangular nets moved to tests/py/unit/fixtures/. 342 passed (was 333); mypy has the same 7 pre-existing errors in 5 files this PR does not touch. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…p the coordinate-snap flag Re-review of 1af2e39 (findings 22.1-22.4): the test-style rewrite had quietly un-pinned two already-fixed defects — the analyzer's frozen model and the absolute hollow-site coordinates — verified by mutation (frozen=False and a 0.2 A hollow offset now fail). interface_displace_film_to_site(film_atom, substrate_atoms=[i, i]) is the only way to name a bridge in a 1x1 cell (the neighbour is the atom's own periodic image); _compact_images_2d collapsed the duplicate to itself (spread 0) and _nearest_neighbour_distance_2d then found no distinct pair to measure, raising "min() arg is an empty sequence". Both now fall back to the atom's nearest non-zero self-image; the same-atom threshold that used to be an unnamed 1e-6 is SAME_ATOM_TOLERANCE. The two analyze/other.py resolvers were tested from the modify-focused file; moved to test_tools_analyze.py next to the rest of that module's tests. Course correction mid-round: center_on_coordinate is removed rather than renamed. get_atom_indices_within_radius_pbc centres on `coordinate` directly whenever one is given, with no snap-to-nearest-atom mode; the coordinate used to be snapped to the nearest atom first (filter_by_sphere's radius-4.5 case on a MoS2 4x4 supercell was 19 atoms snapped, now 18 centred directly — the correct point-at-a-coordinate semantics finding 13.1 originally asked for). get_closest_site_id_from_coordinate_within_radius calls it plainly. test_filter_by_sphere is unaffected: its centre coincides with an atom, so the two behaviours agree on that fixture. Verified: tests/py/unit 347 passed (342 baseline + 6 net new: the sites test's 4 rows, the frozen test, and the new bridge-to-self-image CASES row, minus the one MOS2_4X4 row the removed flag made redundant). pre-commit ruff/black/isort pass; mypy fails with exactly the same 7 pre-existing errors in 5 files untouched by this diff (maxwell_boltzmann.py, lattice_swap_analyzer.py, solid_solution_analyzer.py x3, nanotape/builders.py, nanoribbon/builders.py). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
VsevolodX
marked this pull request as ready for review
September 11, 2026 22:10
…ayer + SurfaceSiteAnalyzer survive
modify.py and analyze/utils.py revert to origin/main: interface_displace_film_to_site and its
helpers/constants/tests are removed — the notebook never imported it, it uses the existing
interface_displace_part. analyze/other.py keeps only get_atom_indices_by_layer as the addition;
get_atom_indices_within_radius_pbc reverts to main's body; get_closest_site_id_from_coordinate_within_radius
and get_atom_indices_in_bottom_layers are gone (duplicates of existing radius/layer lookups, no
consumer of their own). surface_site_analyzer.py is rewritten to the addendum's minimal shape: a
surface: SurfaceTypesEnum = TOP field (reusing the existing enum), the three wrap helpers collapsed
into one, get_in_plane_periodic_images moved in as a private function (its only consumer), and the
docstring cut to five lines. helpers.py exports exactly SurfaceSiteAnalyzer, SurfaceSiteEnum,
get_film_site_occupation, get_atom_indices_by_layer.
Fixtures/mos2.py and fixtures/surface_nets.py (hand-written lattice/basis dicts) and their tests
are deleted; every test material now comes from standata: fixtures/bulk.py gains BULK_Cu, slabs
are built with create_slab from BULK_Ni_PRIMITIVE/BULK_Cu, the MoS2 monolayer from
Materials.get_by_name_and_categories("MoS2", "2D"), and the Gr/Ni(111) interface from
create_interface_zsl over BULK_Ni_PRIMITIVE + BULK_GRAPHENE.
tests/py/unit: 318 passed. pre-commit: clean except the 7 pre-existing mypy errors in untouched
files.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
VsevolodX
changed the title
One parametrized row on test_surface_site_analyzer_sites: atop at the bottom layer's own xy, fcc and hcp swapped versus the top view since BOTTOM counts layers upward from the bottom instead of down from the top. No new fixtures — same NI111_SLAB, same standata-built material. tests/py/unit: 319 passed. pre-commit: clean except the 7 pre-existing mypy errors in untouched files. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…duped test fixtures surface_site_analyzer.py: in_plane_vectors/layers_xy renamed to _in_plane_vectors/_layers_xy (not part of the agreed public surface); tie_tolerance field and the tie branch in get_site_name removed (unreachable at realistic tolerances on every net we test); _wrap rounds only to build the de-duplication key and returns the unrounded, correctly-cornered fractional coordinate, so sites are no longer quantized to 1e-4; the no-op second _wrap call on the atop site is gone; docstrings state what each member returns, not why. test_tools_analyze_surface_site.py: the wrong BOTTOM comment is gone, the sites test asserts set(sites) == set(expected_sites) and pins Ni(111)'s bridge too, the now-redundant Cu(001) fcc/hcp-absence test is folded into that, and the mislabeled "tie" case is named for what it actually tests (off-site). NI111_SLAB/CU001_SLAB/CU110_SLAB/MOS2_MONOLAYER move to tests/py/unit/fixtures/slab.py, shared by both test modules instead of built twice. tests/py/unit: 318 passed. pre-commit: clean except the 7 pre-existing mypy errors in untouched files. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Move the site enum to a package-level enums.py as SurfaceSiteTypesEnum (matching SurfaceTypesEnum), and make sites/get_site_name/ get_displacement_to_site work in 3D crystal coordinates by default like every other analyzer, converting at the boundary with the lattice. Internal Voronoi/distance work stays cartesian.
Args:/Returns: on the three public entry points, matching analyze/other.py; _xy_sites -> _sites_xy for the file's xy-suffix convention; SurfaceSiteAnalyzer and SurfaceSiteTypesEnum filed under helpers.py's Analyzers/Enums __all__ sections instead of Utility Functions.
Measured against origin/main's analyze/ package: only free functions (other.py) carry full Args:/Returns: blocks; analyzer class methods (slab.py, zsl.py, lattice_planes.py, rdf.py) carry none. get_site_name and get_displacement_to_site are class methods -- back to a one-line summary. get_film_site_occupation is a module-level free function like other.py's, so its Args:/Returns:/Raises: block stays.
create_interface_zsl's lattice-match search is not pinned to one outcome across dependency versions (CI saw a 20-atom match vs 6 atoms locally, breaking test_get_film_site_occupation on 3.11.x/3.12.x). Build the substrate and film slabs directly and stack them with create_interface_simple_between_slabs instead, same shape as test_create_simple_interface_between_slabs. The film's own two-atom basis is offset (1/3, 2/3) in the substrate's fractional frame, not the (1/3, 1/3) spacing between named sites, so no rigid xy_shift lands both carbons on named sites at once (verified by hand); shift the film so the first carbon sits exactly atop and pin the honest result. Also drop Final from this file and fixtures/slab.py -- plain module constants match the majority of fixture files; main's three Final test modules are elsewhere.
… building one Building the interface from scratch cannot realize a top+hcp registry: a rigid xy_shift moves both carbons together, so it can only ever place one of them on a named site (verified in the previous commit). The repo already has a prepared, deterministic Gr/Ni(111) top-hcp interface (tests/py/unit/fixtures/interface/gr_ni_111_top_hcp.py, used by test_tools_build_interface.py) -- reuse it and drop the slab-stacking construction it made unnecessary.
VsevolodX
commented
Sep 13, 2026
| return self.material.basis.cell.convert_point_to_crystal(displacement) | ||
|
|
||
|
|
||
| def get_film_site_occupation( |
Member
Author
There was a problem hiding this comment.
Moved: get_film_site_occupation now lives in tools/analyze/crystal_site/helpers.py, matching how made keeps composed convenience functions in a helpers.py beside what they compose (slab/helpers.py → create_slab). The public path is unchanged, from mat3ra.made.tools.helpers import get_film_site_occupation. 773d835.
timurbazhirov
approved these changes
Sep 13, 2026
Restores SUBSTRATE/FILM basis labels on an interface material by element membership, for structures (e.g. after a platform relaxation) that come back without labels. Matches the api-examples helper the review comment was raised against; the api-examples side will call it directly instead of carrying its own copy.
VsevolodX (review, made#298): a composed convenience function belongs in a helpers.py beside what it composes, not in the analyzer module -- same shape as build/pristine_structures/.../slab/helpers.py and analyze/lattice/helpers.py. tools/helpers.py re-exports it from the new location; the public import path is unchanged.
The height of the film atom on the substrate's atop site above the other film atoms -- the quantity papers report as buckling (Lahiri et al., graphene/Ni(111)). Generic for any film/surface and built on get_film_site_occupation, so it lives in the same helpers.py. GRAPHENE_NICKEL_INTERFACE_TOP_HCP is an idealized registry fixture, not a relaxed one: both carbons sit at the same height, so the top-hcp case pins exactly 0.0 rather than a nonzero buckling -- verified against the fixture's own coordinates (both C atoms at fractional z 0.351561882).
A 0.0 pin can't tell mean(atop) - mean(other) apart from the reverse, so the sign -- the whole point of the return value -- was untested. Add a third case: clone the fixture, raise the atop carbon's cartesian z by a known 0.1 A (same clone/to_cartesian/shift/to_crystal pattern interface_displace_part uses), and pin the signed result as that same +0.1 A. No hand-written coordinates.
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
2 participants
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.
Surface sites and layer grouping for adsorbed films
Two additions the specific-examples corpus needs, placed beside the analyzers that already cover
adjacent ground. Everything else this PR previously carried (
interface_displace_film_to_siteandits helpers, the radius/coordinate resolver additions, the synthetic-net fixtures) has been reverted
to
origin/main— no consumer used them; the notebook moves the film with the existinginterface_displace_partand points at atoms with the existingget_closest_site_id_from_coordinate_and_element.SurfaceSiteAnalyzer(tools/analyze/crystal_site/surface_site_analyzer.py) — the namedhigh-symmetry sites of one surface layer (top by default,
surface=BOTTOMwalks the layers fromthe bottom instead): atop, bridge, and the hollows, with three-fold hollows told apart as fcc
/ hcp by which subsurface layer lies beneath. Hollows are the Voronoi vertices of the
periodically tiled surface net, so any lattice and Miller index whose surface is flat works — a
square net's four-fold hollow included, no fcc/hcp on it.
get_site_namereturnsNonewhen apoint is on no site.
get_displacement_to_sitegives the in-plane shift onto the nearest instanceof a named site. Not a duplicate of pymatgen's
AdsorbateSiteFinder: that gives symmetry-reducedontop/bridge/hollow with no fcc/hcp split, where every instance and the fcc/hcp split are the
actual deliverable here.
get_film_site_occupation— which named substrate site each film atom sits on, keyed by atomindex;
Nonewhere no site matches. Raises when the material carries no film-labelled or nosubstrate-labelled atoms.
get_atom_indices_by_layer(tools/analyze/other.py) — atom indices grouped into layers alongz, bottom layer first, independent of basis order.
All exported from
tools.helpers:SurfaceSiteAnalyzer,SurfaceSiteTypesEnum(tools/analyze/crystal_site/enums.py),get_film_site_occupation,get_atom_indices_by_layer. First consumer: the Gr/Ni(111) registryexample (api-examples #364).
Tests: materials come from standata only —
fixtures/bulk.py'sBULK_Cu,BULK_Ni_PRIMITIVE,BULK_GRAPHENE; slabs (Ni(111) 3-layer, Cu(001)/Cu(110) 2-layer) and the MoS2 monolayer(
Materials.get_by_name_and_categories("MoS2", "2D")) built once infixtures/slab.pyand shared byboth test modules; the Gr/Ni(111) interface reuses
GRAPHENE_NICKEL_INTERFACE_TOP_HCP(
fixtures/interface/gr_ni_111_top_hcp.py), the same prepared fixturetest_tools_build_interface.pyuses — deterministic, nothing searched (an earlier revision built it live with
create_interface_zsl; CI on 3.11.x/3.12.x resolved a different lattice match than 3.10.x/local, sothe pinned occupation dict wasn't reproducible). A rigid stack of graphene on Ni(111)
(
create_interface_simple_between_slabs, no search) cannot realise the paper's atop+hollowregistry either: a rigid
xy_shiftmoves both carbons together, so at most one of the two can landon a named site — which is why the test uses the repo's prepared interface instead of building one.
Every site name and coordinate pinned per case (Ni(111)
top and bottom surface: atop/fcc/hcp/bridge; Cu(001): one 4-fold hollow, no fcc/hcp; Cu(110): two
distinct bridge distances; MoS2: atop/hcp/hollow),
get_site_name/get_displacement_to_sitebehaviour,
get_film_site_occupationon the built interface and its twoValueErrorcases, andget_atom_indices_by_layeron the Ni(111)/Cu(001) slabs. Full unit suite green (318). mypy:clean on every file this PR touches; the 7 remaining errors (
lattice_swap_analyzer.py,maxwell_boltzmann.py,solid_solution_analyzer.py×3,nanotape/builders.py,nanoribbon/builders.py) in 5 files are pre-existing and untouched here.Task record: SOF-8043.