feature/SOF-7958 Feat: add NEB calculation notebook for QE neb.x - #349
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ordered and unordered material-set utilities, job payload support, material-set notebooks, NEB image generation and execution notebooks, and convex-hull integration. ChangesMaterials and NEB workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Notebook
participant APIClient
participant Standata
participant Compute
Notebook->>APIClient: authenticate and select account/project
Notebook->>APIClient: load ordered NEB materials
Notebook->>Standata: select and save NEB workflow
Notebook->>APIClient: create and submit NEB job
APIClient->>Compute: execute NEB calculation
Notebook->>APIClient: poll completion and retrieve energy results
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
52d923e to
5490529
Compare
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@other/materials_designer/workflows/neb.ipynb`:
- Around line 118-124: Update the cell 25 logic that computes and applies
effective_n_images so N_IMAGES is used only when the ordered material set
contains exactly the first and last materials; when intermediate saved materials
exist, leave the image count derived from that set and do not write the explicit
N_IMAGES value into the nImages context. Preserve the default fallback of 1 for
the exactly-two-material case.
- Around line 422-443: Update the cluster selection logic around CLUSTER_NAME
and clusters to validate both cases explicitly: raise a clear, actionable error
when the requested name matches no cluster, and when no clusters are available
before accessing clusters[0]. Preserve the existing matching behavior and
compute initialization for valid selections.
- Around line 526-540: Before indexing profile_data in the reaction energy
profile cell, validate that the job completed successfully and that profile_data
contains the expected result; if the job has an error status or the data is
empty, raise a clear failure message instead of allowing an IndexError. Preserve
the existing visualization and peak-energy calculation for successful jobs.
In `@src/py/mat3ra/notebooks_utils/core/entity/material/api.py`:
- Around line 40-53: Escape material_set_name before using it in the regex
filter within find_material_set, importing re and passing
re.escape(material_set_name) so names are matched as literal substrings. Update
tests/py/unit/core/entity/test_material_api.py lines 46-57 to import re and
expect the escaped value in the mock assertion.
In `@tests/py/unit/core/entity/test_material_api.py`:
- Around line 46-57: Update test_find_material_set_returns_first_match so the
expected name regex uses the escaped form of SET_NAME, matching
find_material_set’s re.escape behavior; preserve the remaining mock payload
assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cab0de58-6da1-4818-8b34-99b9a646e838
📒 Files selected for processing (5)
other/materials_designer/workflows/Introduction.ipynbother/materials_designer/workflows/analyze_convex_hull.ipynbother/materials_designer/workflows/neb.ipynbsrc/py/mat3ra/notebooks_utils/core/entity/material/api.pytests/py/unit/core/entity/test_material_api.py
🚧 Files skipped from review as they are similar to previous changes (1)
- other/materials_designer/workflows/Introduction.ipynb
| "source": [ | ||
| "# Intermediate count for QE when the set has only first+last (ignored if middle images exist)\n", | ||
| "N_IMAGES = None # e.g. 20; defaults to 1 when exactly two materials are in the set\n", | ||
| "\n", | ||
| "# K-grid for the NEB unit\n", | ||
| "NEB_KGRID = [1, 1, 1]" | ||
| ] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
N_IMAGES is not actually ignored when intermediate images exist.
Cell id=6's comment states N_IMAGES is "ignored if middle images exist," but the logic in cell id=25 only gates the default fallback (= 1) on len(saved_materials) == 2; an explicitly set N_IMAGES is applied unconditionally via if effective_n_images is not None. If a user sets N_IMAGES while the ordered set also has intermediate images, the workflow's nImages context will be set to that value even though the actual number of images comes from the set, contradicting the documented behavior and potentially producing an inconsistent NEB configuration.
🐛 Proposed fix
effective_n_images = N_IMAGES
if len(saved_materials) == 2 and effective_n_images is None:
effective_n_images = 1
-if effective_n_images is not None:
+if len(saved_materials) == 2 and effective_n_images is not None:
unit_to_modify.add_context(
{"name": "neb", "isEdited": True, "data": {"nImages": effective_n_images}, "extraData": {}}
)
print(f"Using N_IMAGES={effective_n_images}")
+elif effective_n_images is not None:
+ print(f"⚠️ N_IMAGES={effective_n_images} ignored: set already has {len(saved_materials)} images")Also applies to: 342-369
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@other/materials_designer/workflows/neb.ipynb` around lines 118 - 124, Update
the cell 25 logic that computes and applies effective_n_images so N_IMAGES is
used only when the ordered material set contains exactly the first and last
materials; when intermediate saved materials exist, leave the image count
derived from that set and do not write the explicit N_IMAGES value into the
nImages context. Preserve the default fallback of 1 for the exactly-two-material
case.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/py/mat3ra/notebooks_utils/core/entity/material/api.py`:
- Around line 53-59: Escape material_set_name before using it in the regex
filter within find_material_set, importing re in
src/py/mat3ra/notebooks_utils/core/entity/material/api.py and applying
re.escape(). Update tests/py/unit/core/entity/test_material_api.py lines 62-67
to import re and expect re.escape(MATERIAL_SET_NAME) in the mock assertion.
- Around line 154-168: Validate the existing set’s entitySetType in the reuse
branch of the material-set creation flow before assigning or returning
materials_set. Compare materials_set.get("entitySetType") with the type derived
from is_ordered using ORDERED_ENTITY_SET_TYPE or UNORDERED_ENTITY_SET_TYPE; if
they differ, do not silently reuse the set and instead follow the established
error or replacement behavior, preserving the ordering contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec45480a-6b0c-4e37-b619-d7ad91f320c3
📒 Files selected for processing (7)
other/materials_designer/Introduction.ipynbother/materials_designer/create_materials_set.ipynbother/materials_designer/create_neb_path_materials.ipynbother/materials_designer/workflows/analyze_convex_hull.ipynbother/materials_designer/workflows/neb.ipynbsrc/py/mat3ra/notebooks_utils/core/entity/material/api.pytests/py/unit/core/entity/test_material_api.py
🚧 Files skipped from review as they are similar to previous changes (2)
- other/materials_designer/workflows/analyze_convex_hull.ipynb
- other/materials_designer/workflows/neb.ipynb
Escape regex special characters in set name queries (H2+H), pass _materialsSet on NEB job create, and fail clearly when the job does not finish so Cypress matches the UI NEB path. Co-authored-by: Cursor <cursoragent@cursor.com>
| @@ -0,0 +1,229 @@ | |||
| { | |||
There was a problem hiding this comment.
| @@ -0,0 +1,229 @@ | |||
| { | |||
There was a problem hiding this comment.
Let's use HfO2, ferroelectric switch - and use transformation matrix for the final structure generation
Reply via ReviewNB
| @@ -0,0 +1,248 @@ | |||
| { | |||
There was a problem hiding this comment.
| @@ -81,6 +81,7 @@ | |||
| "### 5.2. 2D\n", | |||
There was a problem hiding this comment.
| @@ -0,0 +1,612 @@ | |||
| { | |||
There was a problem hiding this comment.
Resolve src/py/mat3ra/notebooks_utils/core/entity/material/api.py: keep main's bulk-material resolution helpers and its APIClient/Material imports, keep this branch's materials-set helpers, and restore APIClient annotations now that the tests extra pulls in mat3ra-api-client again.
Rename create_neb_path_materials.ipynb -> create_neb_images.ipynb and create_materials_set.ipynb -> utils_create_material_set.ipynb, and update their titles, TOC entries in Introduction.ipynb, and cross-links from neb.ipynb and analyze_convex_hull.ipynb.
- neb.ipynb: correct the N_IMAGES comment — it is applied whenever set, not ignored when the set already holds intermediate images. - neb.ipynb: raise with the available hostnames when CLUSTER_NAME matches no cluster, instead of passing cluster=None into Compute. - material/api.py: refuse to reuse an existing materials set whose entitySetType differs from the requested one, which would silently drop NEB path order.
Blocker — the ordered-set invariant was enforced only where a set is created, not where its order is consumed. find_material_set/list_materials_by_set now take require_ordered, and neb.ipynb passes it on the reuse path: an unordered set has no inSet.index, so every member ties and the 'path' becomes whatever order the API returned. Also: - docstrings on the six new private helpers, including the ordering contract that _index_in_set encodes; - list_materials_in_set takes an already-resolved set, so neb.ipynb resolves it once instead of twice; drop _find_existing_materials_set; - entity-set-type and _materialsSet class constants moved to module top; - _materials_set_reference raises instead of posting an empty slug; - spell out single-letter loop variables in the touched notebook cells; - neb.ipynb: drop the unreachable job-name fallback, and fail with a message when a finished job returns no reaction_energy_profile; - create_neb_images.ipynb: commented-out create_perturbation block moved into prose; - utils_create_material_set.ipynb: f-string brace escaping leaked into markdown. No cells added or removed — neb.feature's cell indices (5, 39, 40) still hold.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/py/mat3ra/notebooks_utils/core/entity/material/api.py`:
- Around line 294-298: Update get_or_create_materials_set around the
find_material_set call to resolve only a materials set whose name exactly equals
material_set_name, rather than accepting the first substring match. Use an
exact-name lookup if available; otherwise reject ambiguous/non-exact matches so
materials are created or added only to the intended set.
- Line 321: Update get_or_create_materials_set before the existing
_move_materials_into_set call to reconcile an existing ordered set: remove or
move out all currently assigned materials, then insert the supplied materials in
the new order. Ensure omitted stale members are cleared and retained members
follow the new ordering, while preserving normal behavior for newly created
sets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a88c40a-bbcf-4c1c-a72e-89be16590810
📒 Files selected for processing (9)
other/materials_designer/Introduction.ipynbother/materials_designer/create_neb_images.ipynbother/materials_designer/utils_create_material_set.ipynbother/materials_designer/workflows/Introduction.ipynbother/materials_designer/workflows/analyze_convex_hull.ipynbother/materials_designer/workflows/neb.ipynbsrc/py/mat3ra/notebooks_utils/core/entity/job/api.pysrc/py/mat3ra/notebooks_utils/core/entity/material/api.pytests/py/unit/core/entity/test_material_api.py
🚧 Files skipped from review as they are similar to previous changes (3)
- other/materials_designer/Introduction.ipynb
- other/materials_designer/workflows/Introduction.ipynb
- other/materials_designer/workflows/analyze_convex_hull.ipynb
| entity_set_type = ORDERED_ENTITY_SET_TYPE if is_ordered else UNORDERED_ENTITY_SET_TYPE | ||
| try: | ||
| materials_set: Optional[Dict[str, Any]] = find_material_set(api_client, owner_id, material_set_name) | ||
| except ValueError: | ||
| materials_set = None |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Resolve an exact set name before reuse.
find_material_set returns the first substring match. If material_set_name is "neb", this path can reuse "neb_previous" instead of creating or using the intended "neb" set. The call then adds materials to the wrong set and can submit the wrong NEB path.
Use an exact-name lookup for get_or_create_materials_set, or reject ambiguous matches.
Proposed fix
def find_material_set(
api_client: APIClient,
owner_id: str,
material_set_name: str,
require_ordered: bool = False,
+ exact_name: bool = False,
) -> Dict[str, Any]:
+ name_pattern = re.escape(material_set_name)
+ if exact_name:
+ name_pattern = f"^{name_pattern}$"
material_sets = api_client.materials.list(
{
"owner._id": owner_id,
"isEntitySet": True,
- "name": {"$regex": re.escape(material_set_name), "$options": "i"},
+ "name": {"$regex": name_pattern, "$options": "i"},
}
)
- materials_set = find_material_set(api_client, owner_id, material_set_name)
+ materials_set = find_material_set(api_client, owner_id, material_set_name, exact_name=True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/py/mat3ra/notebooks_utils/core/entity/material/api.py` around lines 294 -
298, Update get_or_create_materials_set around the find_material_set call to
resolve only a materials set whose name exactly equals material_set_name, rather
than accepting the first substring match. Use an exact-name lookup if available;
otherwise reject ambiguous/non-exact matches so materials are created or added
only to the intended set.
| f"({existing_entity_set_type}, {materials_set['_id']})" | ||
| ) | ||
|
|
||
| _move_materials_into_set(api_client, materials_set["_id"], materials) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect local call sites and tests for entity-set membership replacement behavior.
rg -n -C 4 'move_to_set|remove.*set|inSet|list_materials_in_set|get_or_create_materials_set' src tests otherRepository: mat3ra/api-examples
Length of output: 28465
🏁 Script executed:
#!/usr/bin/env shell
set -euo pipefail
# Inspect the relevant API wrapper and NEB workflow boundaries around reused material sets.
sed -n '220,340p' src/py/mat3ra/notebooks_utils/core/entity/material/api.py
sed -n '236,320p' other/materials_designer/workflows/neb.ipynbRepository: mat3ra/api-examples
Length of output: 7688
🌐 Web query:
mat3ra materials move_to_set endpoint add existing member set replace behavior
💡 Result:
In the Mat3ra platform, the move_to_set endpoint is used to transfer entities (such as materials) from their current location into a designated Set [1][2]. Based on programmatic usage examples, the move_to_set operation is typically called with the following arguments: the ID of the entity to be moved, an empty string (representing the current location or a placeholder for context), and the ID of the destination Set [3]. Regarding the behavior of adding existing members to a Set, the Mat3ra documentation specifies that when an item is moved to a Set, it is no longer present at its original location; it is transferred into the destination Set [1]. There is no documented "replace" behavior for adding existing members; rather, the action is a transfer operation [1]. Users are advised to utilize the API Explorer, available at api-explorer.mat3ra.com, to inspect the specific request schema and response behavior for the move_to_set endpoint in their environment [4].
Citations:
- 1: https://docs.mat3ra.com/entities-general/actions/move-to-sets/
- 2: https://docs.mat3ra.com/entities-general/sets
- 3: https://github.com/Exabyte-io/exabyte-api-examples/blob/master/examples/job/run-simulations-and-extract-properties.ipynb
- 4: https://docs.mat3ra.com/rest-api/api-explorer/
🌐 Web query:
Mat3ra set delete member remove from set API member set
💡 Result:
To remove an item (member) from a Set in the Mat3ra platform via the API, you typically move the item out of the Set or delete the item if it is no longer needed. The Mat3ra API provides a move_to_set functionality [1][2]. Based on official documentation and examples, there is no direct "remove member from set" endpoint. Instead, the standard workflow to remove an entity from a Set is to move it to a different location (such as the main account collection) [1]. 1. Using the Python SDK: In the Mat3ra Python client, entities are moved into sets using the move_to_set method [2]. To effectively "remove" an item, you would move the entity from the current set to the root collection (often represented by an empty string or the parent container ID) [2]. 2. API Explorer: You can use the Mat3ra REST API Explorer to examine available endpoints and test operations for specific entity types (e.g., Materials, Jobs) [3][4]. The platform supports standard RESTful operations, and entity management often relies on updating the parent container or moving resources [5][6]. If you are attempting to remove a member from a Team (which functions differently than an entity set), you navigate to the team's People Explorer and follow the standard delete procedure [7]. For general entity sets, the removal is handled by changing the entity's association [1].
Citations:
- 1: https://docs.mat3ra.com/entities-general/actions/move-to-sets
- 2: https://github.com/Exabyte-io/exabyte-api-examples/blob/master/examples/job/run-simulations-and-extract-properties.ipynb
- 3: https://docs.mat3ra.com/rest-api/api-explorer/
- 4: https://docs.mat3ra.com/rest-api/endpoints/
- 5: https://docs.mat3ra.com/rest-api/overview
- 6: https://docs.mat3ra.com/dev/rest-api/endpoints/
- 7: https://docs.mat3ra.com/collaboration/actions/team/add-remove-member
Clear stale members before reusing an ordered material set.
get_or_create_materials_set only moves the supplied materials into an existing set. If the new ordered list omits or reorders materials that were already members, _move_materials_into_set does not remove them first; rerunning from folder images can therefore keep old images in the path. Move each previously assigned material out before inserting the new ordered list, or expose an explicit replace/reconcile mode for this reuse path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/py/mat3ra/notebooks_utils/core/entity/material/api.py` at line 321,
Update get_or_create_materials_set before the existing _move_materials_into_set
call to reconcile an existing ordered set: remove or move out all currently
assigned materials, then insert the supplied materials in the new order. Ensure
omitted stale members are cleared and retained members follow the new ordering,
while preserving normal behavior for newly created sets.
create_neb_images.ipynb defaulted to bulk Silicon from Standata. The intended example is a surface with an atom displaced out of the surface plane, so load Standata's Si(100) surface (mavrl-si-100, 8 atoms, ~21.9 A cell) instead. Atom 5 is the slab's lowest atom, the one facing the vacuum gap; moving it -0.05 in crystal z (-1.09 A) keeps it inside the cell, so the two images read side by side without an atom wrapping across the periodic boundary. Also give the written images a short PATH_NAME base - filenames come from material names, and the Standata surface name is long and comma-heavy - and point utils_create_material_set.ipynb's default set name at the same example. Verified against the real Standata entry: agents/workdir/tmp/run_create_neb_images.py builds both images, writes them, and reloads them in path order.
ReviewNB on #349, anchored on '### 5.2. 2D': - Introduction.ipynb: the create_neb_images entry is relabelled 'Create initial/final materials' and moves from 5.2 (2D) to 5.1 (3D) as 5.1.2 — displacing an atom out of the surface plane is a 3D perturbation. 5.2.1 and 5.2.2 keep their numbers, so nothing else renumbers. - create_neb_images.ipynb: H1 follows the TOC label. - neb.ipynb: MATERIAL_SET default 'H2+H' -> 'NEB-ordered-material-set'. neb.feature overwrites the whole params cell (cell '5') with its own MATERIAL_SET = 'H2+H', so the test is unaffected. Filename stays create_neb_images.ipynb — that name was TB's own from the previous round, and the comment is on the TOC label, not the file. No cells added or removed; neb.feature anchors 5/39/40 still hold.
The notebook builds an ordered start/end pair from one starting structure; nothing in it is NEB-specific. Say so, rather than framing the whole thing as a Nudged Elastic Band path — NEB is the example and the consumer.
The set section had grown to 249 added lines for what is two operations: find a set and list its members in order, and create-or-reuse a set and move members into it. 119 of those lines were docstrings — a literal reading of my own tb-review TB-DOC-2 finding, applied to private helpers that were one expression each (_exclude_entity_sets was a 2-line comprehension under a 9-line Google-style block). Inline the four one-expression privates into their only call sites, fold the ordered-set guard into find_material_set, and size each docstring to what a reader needs. Nine functions become five; 249 added lines become 144. No behaviour change: the same 38 unit tests pass untouched, and all four public names the notebooks import are unchanged.
Found by running neb.feature against a local platform with no cluster backend registered: client.clusters.list() returns [], and the else branch did 'cluster = clusters[0]' — a bare IndexError with nothing pointing at the cause. The CLUSTER_NAME branch already explained itself; this one did not. No cells added or removed; neb.feature anchors 5/39/40 still hold.
The notebook builds an ordered start/end pair for any calculation — NEB is only the example consumer — so the filename should not say NEB either. Renames the file and updates the links in Introduction.ipynb, neb.ipynb and utils_create_material_set.ipynb. Also drops two prints that only restate what the preceding call did: the 'Wrote N material(s)' line (set_materials already logs each file it writes) and the 'Out-of-plane displacement' line I added alongside the Si(100) example. No cells added or removed; neb.feature anchors 5/39/40 still hold.
'with perturbations' in 8dcd4ca was aspirational — the notebook hand-rolled a loop over set_coordinates. Made has no per-atom move: translate/translate_by_vector move the whole material, and create_perturbation applies f(x,y,z) to every atom through an arc-length normalisation built for sine waves (tested on the Si(100) slab: it displaced all 8 atoms by ~0.28 instead of the target by 0.05). So write the obvious function. translate_atoms(material, indices, vector) moves the named atoms and nothing else, taking ANGSTROM by default — a crystal delta means a different distance in every cell, which is why '-0.05' silently meant 1.09 A here. get_atom_indices_by_height picks the outermost atom so the example survives a change of slab instead of hardcoding index 5. 7 unit tests; the notebook now moves exactly 1.0 A and leaves the other atoms bit-identical. No cells added or removed.
Invented, not asked for. The ask was a function that moves atoms; auto-picking which atom to move was me adding scope. ATOM_INDEX is an explicit 5 again. translate_atoms stays — that is the function.
2 participants
Add espresso NEB workflow notebook
Summary by CodeRabbit