stepped-slab generation with bulk-to-slab atom mapping, fractional filtering, and robust supercell construction#459
stepped-slab generation with bulk-to-slab atom mapping, fractional filtering, and robust supercell construction#459ahmedabdelkawy wants to merge 2 commits intomainfrom
Conversation
…eterministic bulk supercell, track exactly which bulk atom each supercell/slab atom came from, optionally filter atoms in a convenient fractional-coordinate frame, then rotate and add vacuum to obtain the final surface slab. Here S, K, and T are integer direction vectors in the original lattice basis: T is the terrace/surface-normal direction (Miller index of the terrace), while S and K are in-plane directions (step and kink/terrace-width directions) chosen/adjusted to be symmetry-equivalent vectors that lie in the terrace plane. ### Functions added - **`print_fractional_positions(atoms, S, K, T)`**: computes fractional coordinates in a user-defined basis (S, K, T), sorts atoms by depth (`x3`) then in-plane (`x1`, `x2`), and prints them (debug/inspection tool). - **`_hnf_and_U(P)`**: helper to compute the Hermite normal form **H** of an integer supercell matrix **P** and a unimodular transform **U** such that `P = U·H` (used to enumerate translation vectors deterministically). - **`make_supercell(primitive, P)`**: constructs an ASE `Atoms` supercell from an integer transformation matrix **P** with a deterministic atom ordering (“atom-major”), without rotating the lattice. - **`bulk_supercell_with_mapping(...)`**: builds a bulk supercell for stepped/kinked surface construction and returns (i) the supercell, (ii) a “mapping cell”, (iii) fractional coordinates in that mapping cell, and (iv) `orig_index` mapping each supercell atom back to its primitive-cell atom index. - **`apply_fractional_filter(supercell, frac_coords, orig_index, filter_func)`**: applies a user filter in fractional coordinates and returns the filtered structure plus filtered original-index mapping. - **`make_slab(supercell, step_direction, vacuum)`**: rotates the bulk supercell into a slab orientation (step direction aligned in-plane, surface normal as `z`), adds vacuum, and wraps atoms back into the cell.
📝 WalkthroughWalkthroughAdded comprehensive utilities for stepped surface generation, including in-plane direction analysis, supercell construction with atom mapping, fractional coordinate filtering, and slab creation. Introduced nine new functions providing deterministic workflows for building high-index surface structures with explicit orientation and geometry control. No existing code modified. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant create_slab as create_slab()
participant find_inplane as find_inplane_directions()
participant bulk_supercell as bulk_supercell_with_mapping()
participant filter as apply_fractional_filter()
participant make_slab_fn as make_slab()
User->>create_slab: Call with geometry params
create_slab->>find_inplane: Compute step/kink directions
find_inplane-->>create_slab: Return final orientations
create_slab->>bulk_supercell: Build supercell with mapping
bulk_supercell-->>create_slab: Return supercell + index map
alt Filter function provided
create_slab->>filter: Apply atom selection filter
filter-->>create_slab: Return filtered atoms
end
create_slab->>make_slab_fn: Rotate to slab, add vacuum
make_slab_fn-->>create_slab: Return final slab structure
create_slab-->>User: Return slab + index mapping
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
for more information, see https://pre-commit.ci
ahmedabdelkawy
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #459 +/- ##
==========================================
- Coverage 82.91% 76.26% -6.65%
==========================================
Files 25 25
Lines 1820 1993 +173
==========================================
+ Hits 1509 1520 +11
- Misses 311 473 +162 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@jan-janssen, please let me know if this should be added somewhere else and if you have any comments. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/structuretoolkit/build/surface.py (1)
352-365: Replace unconditionalprint(...)calls with logger-based debug outputThese debug prints run in normal library usage and will pollute stdout in downstream workflows.
Proposed refactor
+import logging @@ +logger = logging.getLogger(__name__) @@ - print(f"ncells={n_cells}") + logger.debug("ncells=%s", n_cells) @@ - print(H) - print(U) - print(f"P={P}") - print(h11, h22, h33) - print(f"H U = {H @ U}") + logger.debug("H=%s", H) + logger.debug("U=%s", U) + logger.debug("P=%s", P) + logger.debug("H diagonal: %s %s %s", h11, h22, h33) + logger.debug("H @ U = %s", H @ U) @@ - print(P) cell_super = P @ cell_prim # (3,3) - - print(cell_super) + logger.debug("cell_super=%s", cell_super) @@ - print(f"v2={v2}") + logger.debug("v2=%s", v2)Also applies to: 381-385, 470-470, 814-825
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/structuretoolkit/build/surface.py` around lines 352 - 365, Unconditional print calls around the HNF step (printing n_cells, H, U, P, h11/h22/h33 and H @ U) should be replaced with logger-based debug output: obtain or add a module logger (logging.getLogger(__name__)) and change prints to logger.debug calls (prefer lazy formatting or %s-style interpolation) so debug output is controllable; do the same replacement for the other print sites in this module that print matrix/loop-limit/state information (the ones printing matrices, P, and loop-limit values) to avoid polluting stdout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/structuretoolkit/build/surface.py`:
- Around line 162-178: The function find_inplane_directions currently calls
list(...) on terrace_orientation, step_orientation, and kink_orientation which
can be None; change those conversions to only call list(...) if the value is not
None and otherwise either set a sensible default or raise a clear ValueError.
Specifically, update the initialization of terrace_orientation,
step_orientation, and kink_orientation in find_inplane_directions to do a
conditional conversion (e.g., if terrace_orientation is not None:
terrace_orientation = list(terrace_orientation)) and then validate required
inputs before proceeding so the function no longer crashes on None defaults.
- Around line 789-795: The function returns the pre-filter idxmap instead of the
filtered mapping—after calling apply_fractional_filter(...) which yields slab,
idxmapf, update the return to return the filtered mapping (idxmapf) so the
API/docstring/return annotation are correct; when adjusting the return in the
block that calls make_slab(... S @ bulk_str.cell, vacuum_size) replace the final
returned variable from idxmap to idxmapf (or, if idxmapf must be transformed to
the final slab orientation, apply the same transformation you use for slab
before returning) so the returned index map matches the filtered/rotated slab.
- Around line 296-303: In _hnf_and_U, remove the unreachable bare `raise` and
instead catch the exception (e.g. `except Exception as e:`), then check whether
the input matrix P is already upper‑triangular (use numpy to test P ==
np.triu(P)); if it is, return the safe fallback (np.array(P, dtype=int),
np.eye(3, dtype=int)); otherwise re-raise or raise a new informative error that
includes the original exception info (refer to symbols _hnf_and_U, P, U, H when
making the change).
- Around line 638-641: The vacuum insertion currently writes to the wrong cell
components: in the slab creation code where you call slab.get_cell() (variable
cell) you must zero the x/y components of the third lattice vector (a3) and
extend its z-component by vacuum; change the assignments that now touch
cell[0,2] and cell[1,2] to instead set cell[2,0] = 0.0 and cell[2,1] = 0.0, and
keep the extension as cell[2,2] += float(vacuum) so the row-based cell (rows =
lattice vectors) is updated correctly.
- Around line 769-773: The bug arises because S and K can be reassigned from
numpy arrays to Python sequences when the orthogonality branches set S =
step_orientation or K = kink_orientation, causing the later use of S @
bulk_str.cell to fail; in create_slab ensure any assignment to S or K converts
the value to a numpy array (e.g., via np.asarray or np.array) so S and K always
support ndarray operations, and/or normalize inputs step_orientation and
kink_orientation to numpy arrays at the start of create_slab to guarantee
downstream operations (S @ bulk_str.cell) work correctly.
---
Nitpick comments:
In `@src/structuretoolkit/build/surface.py`:
- Around line 352-365: Unconditional print calls around the HNF step (printing
n_cells, H, U, P, h11/h22/h33 and H @ U) should be replaced with logger-based
debug output: obtain or add a module logger (logging.getLogger(__name__)) and
change prints to logger.debug calls (prefer lazy formatting or %s-style
interpolation) so debug output is controllable; do the same replacement for the
other print sites in this module that print matrix/loop-limit/state information
(the ones printing matrices, P, and loop-limit values) to avoid polluting
stdout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e74a13fd-9ffe-4583-b221-fe2df416c646
📒 Files selected for processing (1)
src/structuretoolkit/build/surface.py
| terrace_orientation=None, | ||
| step_orientation=None, | ||
| kink_orientation=None, | ||
| ): | ||
| """ | ||
| Author: based on code by Shyam Katnagallu | ||
| Find symmetry-equivalent directions of step_direction and kink_direction lying in plane | ||
| given by terrace_orientation | ||
| Args: | ||
| basis: some atomic structure | ||
| terrace_orientation (list): The miller index of the terrace eg., [1,1,1] | ||
| step_orientation (list): The miller index of the step eg., [1,1,0] | ||
| kink_orientation (list): The miller index of the kink eg., [1,1,1] | ||
| """ | ||
| terrace_orientation = list(terrace_orientation) | ||
| step_orientation = list(step_orientation) | ||
| kink_orientation = list(kink_orientation) |
There was a problem hiding this comment.
find_inplane_directions defaults currently crash on None inputs
Line 176–178 call list(...) on possibly None values from the function defaults, causing a TypeError before any validation.
Proposed fix
def find_inplane_directions(
basis,
terrace_orientation=None,
step_orientation=None,
kink_orientation=None,
):
@@
- terrace_orientation = list(terrace_orientation)
- step_orientation = list(step_orientation)
- kink_orientation = list(kink_orientation)
+ if terrace_orientation is None:
+ terrace_orientation = [1, 1, 1]
+ if step_orientation is None:
+ step_orientation = [1, 1, 0]
+ if kink_orientation is None:
+ kink_orientation = [1, 1, 1]
+ terrace_orientation = list(terrace_orientation)
+ step_orientation = list(step_orientation)
+ kink_orientation = list(kink_orientation)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| terrace_orientation=None, | |
| step_orientation=None, | |
| kink_orientation=None, | |
| ): | |
| """ | |
| Author: based on code by Shyam Katnagallu | |
| Find symmetry-equivalent directions of step_direction and kink_direction lying in plane | |
| given by terrace_orientation | |
| Args: | |
| basis: some atomic structure | |
| terrace_orientation (list): The miller index of the terrace eg., [1,1,1] | |
| step_orientation (list): The miller index of the step eg., [1,1,0] | |
| kink_orientation (list): The miller index of the kink eg., [1,1,1] | |
| """ | |
| terrace_orientation = list(terrace_orientation) | |
| step_orientation = list(step_orientation) | |
| kink_orientation = list(kink_orientation) | |
| terrace_orientation=None, | |
| step_orientation=None, | |
| kink_orientation=None, | |
| ): | |
| """ | |
| Author: based on code by Shyam Katnagallu | |
| Find symmetry-equivalent directions of step_direction and kink_direction lying in plane | |
| given by terrace_orientation | |
| Args: | |
| basis: some atomic structure | |
| terrace_orientation (list): The miller index of the terrace eg., [1,1,1] | |
| step_orientation (list): The miller index of the step eg., [1,1,0] | |
| kink_orientation (list): The miller index of the kink eg., [1,1,1] | |
| """ | |
| if terrace_orientation is None: | |
| terrace_orientation = [1, 1, 1] | |
| if step_orientation is None: | |
| step_orientation = [1, 1, 0] | |
| if kink_orientation is None: | |
| kink_orientation = [1, 1, 1] | |
| terrace_orientation = list(terrace_orientation) | |
| step_orientation = list(step_orientation) | |
| kink_orientation = list(kink_orientation) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/structuretoolkit/build/surface.py` around lines 162 - 178, The function
find_inplane_directions currently calls list(...) on terrace_orientation,
step_orientation, and kink_orientation which can be None; change those
conversions to only call list(...) if the value is not None and otherwise either
set a sensible default or raise a clear ValueError. Specifically, update the
initialization of terrace_orientation, step_orientation, and kink_orientation in
find_inplane_directions to do a conditional conversion (e.g., if
terrace_orientation is not None: terrace_orientation =
list(terrace_orientation)) and then validate required inputs before proceeding
so the function no longer crashes on None defaults.
| except Exception: | ||
| raise | ||
| # pragma: no cover (covers missing SymPy or old version) | ||
| # Very simple fallback: assume P is already upper‑triangular (or diagonal) | ||
| # In that case U = identity and H = P. | ||
| # This will still produce a correct super‑cell, only the enumeration | ||
| # of translation vectors may be less compact. | ||
| return np.array(P, dtype=int), np.eye(3, dtype=int) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/structuretoolkit/build/surface.py | sed -n '290,310p'Repository: pyiron/structuretoolkit
Length of output: 1029
Fallback in _hnf_and_U is unreachable
Line 297 re-raises immediately, making the documented fallback at lines 299–303 completely inaccessible. The bare raise statement exits the exception handler before the fallback code can execute.
The proposed fix correctly restructures the exception handler to capture the exception, check if the matrix is already upper-triangular, and return the safe fallback only when applicable. Otherwise, it raises a more informative error message.
Proposed fix
- except Exception:
- raise
- # pragma: no cover (covers missing SymPy or old version)
- # Very simple fallback: assume P is already upper‑triangular (or diagonal)
- # In that case U = identity and H = P.
- # This will still produce a correct super‑cell, only the enumeration
- # of translation vectors may be less compact.
- return np.array(P, dtype=int), np.eye(3, dtype=int)
+ except Exception as err:
+ # pragma: no cover (missing SymPy / incompatible SymPy)
+ P_int = np.array(P, dtype=int)
+ if np.all(np.tril(P_int, k=-1) == 0):
+ return P_int, np.eye(3, dtype=int)
+ raise RuntimeError(
+ "SymPy is required for non-upper-triangular transformation matrices."
+ ) from err📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception: | |
| raise | |
| # pragma: no cover (covers missing SymPy or old version) | |
| # Very simple fallback: assume P is already upper‑triangular (or diagonal) | |
| # In that case U = identity and H = P. | |
| # This will still produce a correct super‑cell, only the enumeration | |
| # of translation vectors may be less compact. | |
| return np.array(P, dtype=int), np.eye(3, dtype=int) | |
| except Exception as err: | |
| # pragma: no cover (missing SymPy / incompatible SymPy) | |
| P_int = np.array(P, dtype=int) | |
| if np.all(np.tril(P_int, k=-1) == 0): | |
| return P_int, np.eye(3, dtype=int) | |
| raise RuntimeError( | |
| "SymPy is required for non-upper-triangular transformation matrices." | |
| ) from err |
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 296-303: Remove exception handler; error is immediately re-raised
(TRY203)
[warning] 299-299: Comment contains ambiguous ‑ (NON-BREAKING HYPHEN). Did you mean - (HYPHEN-MINUS)?
(RUF003)
[warning] 301-301: Comment contains ambiguous ‑ (NON-BREAKING HYPHEN). Did you mean - (HYPHEN-MINUS)?
(RUF003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/structuretoolkit/build/surface.py` around lines 296 - 303, In _hnf_and_U,
remove the unreachable bare `raise` and instead catch the exception (e.g.
`except Exception as e:`), then check whether the input matrix P is already
upper‑triangular (use numpy to test P == np.triu(P)); if it is, return the safe
fallback (np.array(P, dtype=int), np.eye(3, dtype=int)); otherwise re-raise or
raise a new informative error that includes the original exception info (refer
to symbols _hnf_and_U, P, U, H when making the change).
| cell = slab.get_cell() # rows again | ||
| cell[0, 2] = 0.0 # a3_x → 0 | ||
| cell[1, 2] = 0.0 # a3_y → 0 | ||
| cell[2, 2] += float(vacuum) # extend along z |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the file around the mentioned lines to see the context
sed -n '630,650p' src/structuretoolkit/build/surface.pyRepository: pyiron/structuretoolkit
Length of output: 1033
🏁 Script executed:
# Get a broader view of the function containing these lines
sed -n '600,650p' src/structuretoolkit/build/surface.pyRepository: pyiron/structuretoolkit
Length of output: 2507
🏁 Script executed:
# Find the function definition that contains these lines
rg -B 50 "cell\[0, 2\] = 0.0" src/structuretoolkit/build/surface.py | head -100Repository: pyiron/structuretoolkit
Length of output: 2499
Vacuum insertion edits the wrong cell components
The code at lines 639–640 modifies cell[0,2] and cell[1,2] (the z-components of lattice vectors a1 and a2), but the intent stated in the comment is to zero the x and y components of the third lattice vector a3. Since the cell matrix uses row-based representation (where rows are lattice vectors and columns are components: [x, y, z]), this should target cell[2,0] and cell[2,1] instead.
Proposed fix
- cell[0, 2] = 0.0 # a3_x → 0
- cell[1, 2] = 0.0 # a3_y → 0
- cell[2, 2] += float(vacuum) # extend along z
+ cell[2, 0] = 0.0 # a3_x -> 0
+ cell[2, 1] = 0.0 # a3_y -> 0
+ cell[2, 2] += float(vacuum) # extend along z📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cell = slab.get_cell() # rows again | |
| cell[0, 2] = 0.0 # a3_x → 0 | |
| cell[1, 2] = 0.0 # a3_y → 0 | |
| cell[2, 2] += float(vacuum) # extend along z | |
| cell = slab.get_cell() # rows again | |
| cell[2, 0] = 0.0 # a3_x -> 0 | |
| cell[2, 1] = 0.0 # a3_y -> 0 | |
| cell[2, 2] += float(vacuum) # extend along z |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/structuretoolkit/build/surface.py` around lines 638 - 641, The vacuum
insertion currently writes to the wrong cell components: in the slab creation
code where you call slab.get_cell() (variable cell) you must zero the x/y
components of the third lattice vector (a3) and extend its z-component by
vacuum; change the assignments that now touch cell[0,2] and cell[1,2] to instead
set cell[2,0] = 0.0 and cell[2,1] = 0.0, and keep the extension as cell[2,2] +=
float(vacuum) so the row-based cell (rows = lattice vectors) is updated
correctly.
| if np.dot(step_orientation, terrace_orientation) == 0: | ||
| S = step_orientation | ||
| if np.dot(kink_orientation, terrace_orientation) == 0: | ||
| K = kink_orientation | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and get its size
wc -l src/structuretoolkit/build/surface.pyRepository: pyiron/structuretoolkit
Length of output: 108
🏁 Script executed:
# Now let's examine the specific lines mentioned in the review
sed -n '760,800p' src/structuretoolkit/build/surface.py | cat -nRepository: pyiron/structuretoolkit
Length of output: 1575
🏁 Script executed:
# Let's also check the broader context - the function definition and variable initialization
sed -n '750,810p' src/structuretoolkit/build/surface.py | cat -nRepository: pyiron/structuretoolkit
Length of output: 2141
🏁 Script executed:
# Let's see the full function signature to understand parameter types
sed -n '700,770p' src/structuretoolkit/build/surface.py | cat -nRepository: pyiron/structuretoolkit
Length of output: 3007
🏁 Script executed:
# Let's check what find_inplane_directions returns
rg -A 20 "def find_inplane_directions" src/structuretoolkit/build/surface.pyRepository: pyiron/structuretoolkit
Length of output: 853
🏁 Script executed:
# Let's check if there are any type hints in the file
rg -B 5 -A 30 "def create_slab" src/structuretoolkit/build/surface.py | head -50Repository: pyiron/structuretoolkit
Length of output: 1165
🏁 Script executed:
# Let's test if tuple @ numpy.array actually raises TypeError
python << 'EOF'
import numpy as np
# Test case 1: tuple @ numpy array
try:
tuple_val = (1, 0, 0)
array_val = np.array([[1, 0, 0], [0, 1, 0], [0, 0, 1]], dtype=float)
result = tuple_val @ array_val
print("tuple @ array succeeded:", result)
except TypeError as e:
print("tuple @ array failed with TypeError:", e)
# Test case 2: list @ numpy array
try:
list_val = [1, 0, 0]
array_val = np.array([[1, 0, 0], [0, 1, 0], [0, 0, 1]], dtype=float)
result = list_val @ array_val
print("list @ array succeeded:", result)
except TypeError as e:
print("list @ array failed with TypeError:", e)
# Test case 3: numpy array @ numpy array
try:
arr_val = np.array([1, 0, 0], dtype=int)
array_val = np.array([[1, 0, 0], [0, 1, 0], [0, 0, 1]], dtype=float)
result = arr_val @ array_val
print("numpy array @ array succeeded:", result)
except TypeError as e:
print("numpy array @ array failed with TypeError:", e)
EOFRepository: pyiron/structuretoolkit
Length of output: 184
🏁 Script executed:
# Let me check what find_inplane_directions actually returns
rg -A 50 "def find_inplane_directions" src/structuretoolkit/build/surface.py | head -80Repository: pyiron/structuretoolkit
Length of output: 2140
🏁 Script executed:
# Check the return statement of find_inplane_directions
rg -B 5 "return.*step_direction\|return.*kink_direction" src/structuretoolkit/build/surface.py | head -30Repository: pyiron/structuretoolkit
Length of output: 49
🏁 Script executed:
# Get the complete find_inplane_directions function
sed -n '560,650p' src/structuretoolkit/build/surface.py | cat -nRepository: pyiron/structuretoolkit
Length of output: 4656
🏁 Script executed:
# Check if there are any imports and what bulk_str is (it's ase.Atoms)
head -30 src/structuretoolkit/build/surface.py | grep -E "^import|^from"Repository: pyiron/structuretoolkit
Length of output: 414
🏁 Script executed:
# Verify the behavior difference - tuple @ array would fail, array @ array works
# Since we don't have numpy, let's check ASE usage
rg "bulk_str\.cell" src/structuretoolkit/build/surface.py | head -10Repository: pyiron/structuretoolkit
Length of output: 221
create_slab fails at runtime when S/K are reassigned from sequences
Lines 770–773 can reassign S/K from numpy arrays to tuples or lists if the orthogonality condition is true. Line 792 then performs S @ bulk_str.cell, which raises TypeError because the @ operator is not defined for tuples/lists. This occurs when step_orientation or kink_orientation are passed as sequences (common case) and happen to be orthogonal to the terrace.
Proposed fix
if np.dot(step_orientation, terrace_orientation) == 0:
- S = step_orientation
+ S = np.asarray(step_orientation, dtype=int)
if np.dot(kink_orientation, terrace_orientation) == 0:
- K = kink_orientation
+ K = np.asarray(kink_orientation, dtype=int)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if np.dot(step_orientation, terrace_orientation) == 0: | |
| S = step_orientation | |
| if np.dot(kink_orientation, terrace_orientation) == 0: | |
| K = kink_orientation | |
| if np.dot(step_orientation, terrace_orientation) == 0: | |
| S = np.asarray(step_orientation, dtype=int) | |
| if np.dot(kink_orientation, terrace_orientation) == 0: | |
| K = np.asarray(kink_orientation, dtype=int) | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/structuretoolkit/build/surface.py` around lines 769 - 773, The bug arises
because S and K can be reassigned from numpy arrays to Python sequences when the
orthogonality branches set S = step_orientation or K = kink_orientation, causing
the later use of S @ bulk_str.cell to fail; in create_slab ensure any assignment
to S or K converts the value to a numpy array (e.g., via np.asarray or np.array)
so S and K always support ndarray operations, and/or normalize inputs
step_orientation and kink_orientation to numpy arrays at the start of
create_slab to guarantee downstream operations (S @ bulk_str.cell) work
correctly.
| slab, idxmapf = apply_fractional_filter(supercell, frac, idxmap, filter_function) | ||
|
|
||
| # rotate cell into final orientation | ||
| slab = make_slab(slab, S @ bulk_str.cell, vacuum_size) | ||
|
|
||
| return slab, idxmap | ||
|
|
There was a problem hiding this comment.
Returned index mapping is incorrect after filtering
Line 789 computes idxmapf (filtered mapping), but Line 794 returns idxmap (pre-filter mapping). This breaks the API contract in the docstring and return annotation.
Proposed fix
- slab, idxmapf = apply_fractional_filter(supercell, frac, idxmap, filter_function)
+ slab, idxmapf = apply_fractional_filter(supercell, frac, idxmap, filter_function)
@@
- return slab, idxmap
+ return slab, {i: int(orig_i) for i, orig_i in enumerate(idxmapf)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| slab, idxmapf = apply_fractional_filter(supercell, frac, idxmap, filter_function) | |
| # rotate cell into final orientation | |
| slab = make_slab(slab, S @ bulk_str.cell, vacuum_size) | |
| return slab, idxmap | |
| slab, idxmapf = apply_fractional_filter(supercell, frac, idxmap, filter_function) | |
| # rotate cell into final orientation | |
| slab = make_slab(slab, S @ bulk_str.cell, vacuum_size) | |
| return slab, {i: int(orig_i) for i, orig_i in enumerate(idxmapf)} |
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 789-789: Unpacked variable idxmapf is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/structuretoolkit/build/surface.py` around lines 789 - 795, The function
returns the pre-filter idxmap instead of the filtered mapping—after calling
apply_fractional_filter(...) which yields slab, idxmapf, update the return to
return the filtered mapping (idxmapf) so the API/docstring/return annotation are
correct; when adjusting the return in the block that calls make_slab(... S @
bulk_str.cell, vacuum_size) replace the final returned variable from idxmap to
idxmapf (or, if idxmapf must be transformed to the final slab orientation, apply
the same transformation you use for slab before returning) so the returned index
map matches the filtered/rotated slab.
Hi @ahmedabdelkawy , thanks a lot for the contribution. Can you add unit tests for your function? |
|
HI @jan-janssen I dont have any experience there tbh, and @freyso wrote the entire code. I believe it will take some, unless it is LLM generated 😅 |
I will take care of that (certainly LLM-supported). |
3 participants
These additions refactor slab construction into clear steps—build a deterministic bulk supercell, track exactly which bulk atom each supercell/slab atom came from, optionally filter atoms in a convenient fractional-coordinate frame, then rotate and add vacuum to obtain the final surface slab.
Here S, K, and T are integer direction vectors in the original lattice basis: T is the terrace/surface-normal direction (Miller index of the terrace), while S and K are in-plane directions (step and kink/terrace-width directions) chosen/adjusted to be symmetry-equivalent vectors that lie in the terrace plane.
Functions added
print_fractional_positions(atoms, S, K, T): computes fractional coordinates in a user-defined basis (S, K, T), sorts atoms by depth (x3) then in-plane (x1,x2), and prints them (debug/inspection tool)._hnf_and_U(P): helper to compute the Hermite normal form H of an integer supercell matrix P and a unimodular transform U such thatP = U·H(used to enumerate translation vectors deterministically).make_supercell(primitive, P): constructs an ASEAtomssupercell from an integer transformation matrix P with a deterministic atom ordering (“atom-major”), without rotating the lattice.bulk_supercell_with_mapping(...): builds a bulk supercell for stepped/kinked surface construction and returns (i) the supercell, (ii) a “mapping cell”, (iii) fractional coordinates in that mapping cell, and (iv)orig_indexmapping each supercell atom back to its primitive-cell atom index.apply_fractional_filter(supercell, frac_coords, orig_index, filter_func): applies a user filter in fractional coordinates and returns the filtered structure plus filtered original-index mapping.make_slab(supercell, step_direction, vacuum): rotates the bulk supercell into a slab orientation (step direction aligned in-plane, surface normal asz), adds vacuum, and wraps atoms back into the cell.Summary by CodeRabbit