feature/SOF-7958 Feat: add NEB calculation notebook for QE neb.x#349
Open
VsevolodX wants to merge 13 commits into
Open
feature/SOF-7958 Feat: add NEB calculation notebook for QE neb.x#349VsevolodX wants to merge 13 commits into
VsevolodX wants to merge 13 commits into
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughChangesAdds reusable material-set lookup and creation APIs with tests, notebooks for creating sets and generating NEB path materials, convex-hull integration, and a linked Quantum ESPRESSO NEB workflow covering setup, submission, polling, and result visualization. Materials and NEB workflow
Estimated code review effort: 4 (Complex) | ~60 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 | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
VsevolodX
force-pushed
the
feature/SOF-7958
branch
from
July 17, 2026 18:19
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
Comment on lines
+118
to
+124
| "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.
Comment on lines
+422
to
+443
| "cell_type": "code", | ||
| "execution_count": null, | ||
| "id": "31", | ||
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "from mat3ra.ide.compute import Compute\n", | ||
| "\n", | ||
| "# Select cluster: use specified name if provided, otherwise use first available\n", | ||
| "if CLUSTER_NAME:\n", | ||
| " cluster = next((c for c in clusters if CLUSTER_NAME in c[\"hostname\"]), None)\n", | ||
| "else:\n", | ||
| " cluster = clusters[0]\n", | ||
| "\n", | ||
| "compute = Compute(\n", | ||
| " cluster=cluster,\n", | ||
| " queue=QUEUE_NAME,\n", | ||
| " ppn=PPN\n", | ||
| ")\n", | ||
| "print(f\"Using cluster: {compute.cluster.hostname}, queue: {QUEUE_NAME}, ppn: {PPN}\")" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unhelpful failure when CLUSTER_NAME doesn't match, or when no clusters exist.
If CLUSTER_NAME is set but matches no cluster, cluster silently becomes None, and the later compute.cluster.hostname reference will raise an unclear AttributeError. Similarly, clusters[0] in the else-branch will raise an IndexError if the account/project has no clusters. Both should fail with a clear, actionable message.
🛡️ Proposed fix
+if not clusters:
+ raise ValueError("No clusters available for this account/project.")
+
if CLUSTER_NAME:
cluster = next((c for c in clusters if CLUSTER_NAME in c["hostname"]), None)
+ if cluster is None:
+ raise ValueError(
+ f"No cluster matching '{CLUSTER_NAME}'. Available: {[c['hostname'] for c in clusters]}"
+ )
else:
cluster = clusters[0]📝 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.
Suggested change
| "cell_type": "code", | |
| "execution_count": null, | |
| "id": "31", | |
| "metadata": {}, | |
| "outputs": [], | |
| "source": [ | |
| "from mat3ra.ide.compute import Compute\n", | |
| "\n", | |
| "# Select cluster: use specified name if provided, otherwise use first available\n", | |
| "if CLUSTER_NAME:\n", | |
| " cluster = next((c for c in clusters if CLUSTER_NAME in c[\"hostname\"]), None)\n", | |
| "else:\n", | |
| " cluster = clusters[0]\n", | |
| "\n", | |
| "compute = Compute(\n", | |
| " cluster=cluster,\n", | |
| " queue=QUEUE_NAME,\n", | |
| " ppn=PPN\n", | |
| ")\n", | |
| "print(f\"Using cluster: {compute.cluster.hostname}, queue: {QUEUE_NAME}, ppn: {PPN}\")" | |
| ] | |
| }, | |
| "cell_type": "code", | |
| "execution_count": null, | |
| "id": "31", | |
| "metadata": {}, | |
| "outputs": [], | |
| "source": [ | |
| "from mat3ra.ide.compute import Compute\n", | |
| "\n", | |
| "# Select cluster: use specified name if provided, otherwise use first available\n", | |
| "if not clusters:\n", | |
| " raise ValueError(\"No clusters available for this account/project.\")\n", | |
| "\n", | |
| "if CLUSTER_NAME:\n", | |
| " cluster = next((c for c in clusters if CLUSTER_NAME in c[\"hostname\"]), None)\n", | |
| " if cluster is None:\n", | |
| " raise ValueError(\n", | |
| " f\"No cluster matching '{CLUSTER_NAME}'. Available: {[c['hostname'] for c in clusters]}\"\n", | |
| " )\n", | |
| "else:\n", | |
| " cluster = clusters[0]\n", | |
| "\n", | |
| "compute = Compute(\n", | |
| " cluster=cluster,\n", | |
| " queue=QUEUE_NAME,\n", | |
| " ppn=PPN\n", | |
| ")\n", | |
| "print(f\"Using cluster: {compute.cluster.hostname}, queue: {QUEUE_NAME}, ppn: {PPN}\")" | |
| ] | |
| }, |
🤖 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 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.
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
Comment on lines
+154
to
+168
| materials_set = _find_existing_materials_set(api_client, owner_id, material_set_name) | ||
| if materials_set is None: | ||
| entity_set_type = ORDERED_ENTITY_SET_TYPE if is_ordered else UNORDERED_ENTITY_SET_TYPE | ||
| set_config = { | ||
| "name": material_set_name, | ||
| "owner": {"_id": owner_id}, | ||
| "entitySetType": entity_set_type, | ||
| } | ||
| materials_set = api_client.materials.create_set(set_config) | ||
| print(f"✅ Materials set '{materials_set['name']}' " f"({entity_set_type}, {materials_set['_id']})") | ||
| else: | ||
| print( | ||
| f"♻️ Reusing existing materials set '{materials_set['name']}' " | ||
| f"({materials_set.get('entitySetType')}, {materials_set['_id']})" | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reused set's entitySetType isn't validated against the requested is_ordered.
When an existing set is found by name, the code reuses it unconditionally without checking whether materials_set.get("entitySetType") matches the requested is_ordered. An unordered set (e.g. convex-hull) reused for an ordered NEB path (or vice versa) would silently break the path-ordering contract that downstream NEB notebooks depend on (inSet.index).
🛡️ Proposed fix
materials_set = _find_existing_materials_set(api_client, owner_id, material_set_name)
if materials_set is None:
entity_set_type = ORDERED_ENTITY_SET_TYPE if is_ordered else UNORDERED_ENTITY_SET_TYPE
set_config = {
"name": material_set_name,
"owner": {"_id": owner_id},
"entitySetType": entity_set_type,
}
materials_set = api_client.materials.create_set(set_config)
print(f"✅ Materials set '{materials_set['name']}' " f"({entity_set_type}, {materials_set['_id']})")
else:
+ expected_entity_set_type = ORDERED_ENTITY_SET_TYPE if is_ordered else UNORDERED_ENTITY_SET_TYPE
+ if materials_set.get("entitySetType") != expected_entity_set_type:
+ raise ValueError(
+ f"Materials set '{material_set_name}' exists as "
+ f"'{materials_set.get('entitySetType')}', but is_ordered={is_ordered} was requested."
+ )
print(
f"♻️ Reusing existing materials set '{materials_set['name']}' "
f"({materials_set.get('entitySetType')}, {materials_set['_id']})"
)📝 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.
Suggested change
| materials_set = _find_existing_materials_set(api_client, owner_id, material_set_name) | |
| if materials_set is None: | |
| entity_set_type = ORDERED_ENTITY_SET_TYPE if is_ordered else UNORDERED_ENTITY_SET_TYPE | |
| set_config = { | |
| "name": material_set_name, | |
| "owner": {"_id": owner_id}, | |
| "entitySetType": entity_set_type, | |
| } | |
| materials_set = api_client.materials.create_set(set_config) | |
| print(f"✅ Materials set '{materials_set['name']}' " f"({entity_set_type}, {materials_set['_id']})") | |
| else: | |
| print( | |
| f"♻️ Reusing existing materials set '{materials_set['name']}' " | |
| f"({materials_set.get('entitySetType')}, {materials_set['_id']})" | |
| ) | |
| materials_set = _find_existing_materials_set(api_client, owner_id, material_set_name) | |
| if materials_set is None: | |
| entity_set_type = ORDERED_ENTITY_SET_TYPE if is_ordered else UNORDERED_ENTITY_SET_TYPE | |
| set_config = { | |
| "name": material_set_name, | |
| "owner": {"_id": owner_id}, | |
| "entitySetType": entity_set_type, | |
| } | |
| materials_set = api_client.materials.create_set(set_config) | |
| print(f"✅ Materials set '{materials_set['name']}' " f"({entity_set_type}, {materials_set['_id']})") | |
| else: | |
| expected_entity_set_type = ORDERED_ENTITY_SET_TYPE if is_ordered else UNORDERED_ENTITY_SET_TYPE | |
| if materials_set.get("entitySetType") != expected_entity_set_type: | |
| raise ValueError( | |
| f"Materials set '{material_set_name}' exists as " | |
| f"'{materials_set.get('entitySetType')}', but is_ordered={is_ordered} was requested." | |
| ) | |
| print( | |
| f"♻️ Reusing existing materials set '{materials_set['name']}' " | |
| f"({materials_set.get('entitySetType')}, {materials_set['_id']})" | |
| ) |
🤖 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 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.
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>
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
1 participant
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.
Add espresso NEB workflow notebook
Summary by CodeRabbit