Update: return ZPE NB - #345
Merged
Merged
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
📝 WalkthroughWalkthroughAdds a zero-point energy workflow notebook covering material setup, Standata workflow construction, compute configuration, job execution, and result visualization. Updates the workflow index with links to surface-energy and zero-point-energy notebooks and adds Python notebook metadata. ChangesZero-point energy workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Notebook
participant PlatformAPI
participant ComputeJob
User->>Notebook: configure ZPE workflow parameters
Notebook->>PlatformAPI: authenticate and prepare material
Notebook->>PlatformAPI: create and save workflow
Notebook->>PlatformAPI: create and submit job
PlatformAPI->>ComputeJob: execute SCF and DFPT steps
ComputeJob-->>PlatformAPI: completed job properties
Notebook->>PlatformAPI: retrieve zero-point energy
PlatformAPI-->>Notebook: scalar ZPE result
Notebook-->>User: visualize result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Ruff (0.15.21)other/materials_designer/workflows/Introduction.ipynbUnexpected end of JSON input other/materials_designer/workflows/zero_point_energy.ipynbUnexpected end of JSON input 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/zero_point_energy.ipynb`:
- Around line 233-235: Update the project selection flow around
client.projects.list and project_id so it checks whether the returned projects
collection contains a default project before indexing projects[0]. If none
exists, raise or report an actionable configuration error identifying the
selected account and the need to create or configure a default project; preserve
the existing project_id assignment and success message when a project is
available.
- Around line 498-509: Update the cluster selection logic before constructing
Compute to validate that clusters is non-empty and that a specified CLUSTER_NAME
matches an available cluster. Raise a clear error for either missing
availability or an unmatched name, and only pass a confirmed cluster to Compute
and the subsequent print statement.
- Around line 579-581: Update the wait_for_jobs_to_finish_async call in the
zero-point energy workflow to remove the unsupported poll_interval argument,
passing only client.jobs and [job_id] as required by the function signature.
- Around line 10-18: Update the zero-point energy notebook description to
identify the result as a Γ-point approximation rather than bulk or converged
ZPE. Either rename result labels and explanatory text accordingly, or replace
the Γ-only phonon workflow with a Brillouin-zone q-point workflow that computes
converged bulk ZPE.
🪄 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: 611245a1-2570-4ef2-b5cc-629fd22ba7c8
📒 Files selected for processing (2)
other/materials_designer/workflows/Introduction.ipynbother/materials_designer/workflows/zero_point_energy.ipynb
Comment on lines
+10
to
+18
| "Calculate the zero-point energy (ZPE) of a material using a DFT phonon workflow on the Mat3ra platform.\n", | ||
| "\n", | ||
| "The ZPE is computed from a gamma-point phonon calculation:\n", | ||
| "$$\\text{ZPE} = \\frac{1}{2} \\sum_i \\hbar\\omega_i$$\n", | ||
| "where $\\omega_i$ are the phonon frequencies at the Γ-point.\n", | ||
| "\n", | ||
| "The workflow runs two steps:\n", | ||
| "1. **SCF** (`pw.x`): Self-consistent field calculation to obtain the ground-state charge density.\n", | ||
| "2. **Phonon at Γ** (`ph.x`): Density-functional perturbation theory (DFPT) calculation at the gamma point to obtain phonon frequencies and ZPE.\n", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the referenced Standata workflow for q-point/grid configuration.
fd -a 'zero_point_energy.*json' . |
while read -r file; do
echo "=== $file ==="
rg -n -C3 'qpoint|q-point|grid|gamma|phonon|ph\.x' "$file"
doneRepository: mat3ra/api-examples
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files matching zero_point_energy =="
fd -a 'zero_point_energy' .
echo
echo "== notebook outline =="
ast-grep outline other/materials_designer/workflows/zero_point_energy.ipynb --view expanded || true
echo
echo "== notebook text around ZPE mentions =="
python3 - <<'PY'
import json
from pathlib import Path
p = Path('other/materials_designer/workflows/zero_point_energy.ipynb')
nb = json.loads(p.read_text())
for i, cell in enumerate(nb.get('cells', [])):
if cell.get('cell_type') == 'markdown':
src = ''.join(cell.get('source', []))
if 'zero-point' in src.lower() or 'zpe' in src.lower() or 'gamma' in src.lower() or 'phonon' in src.lower():
print(f"\n--- markdown cell {i} ---")
print(src)
elif cell.get('cell_type') == 'code':
src = ''.join(cell.get('source', []))
if 'zpe' in src.lower() or 'phonon' in src.lower() or 'gamma' in src.lower():
print(f"\n--- code cell {i} ---")
print(src)
PY
echo
echo "== search for workflow definition references =="
rg -n -i 'zero_point_energy|zpe|gamma-point|gamma point|q-point|qpoint|phonon' other/materials_designer -g '!**/*.ipynb' || trueRepository: mat3ra/api-examples
Length of output: 3958
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json
from pathlib import Path
p = Path('other/materials_designer/workflows/zero_point_energy.ipynb')
nb = json.loads(p.read_text())
terms = ['approx', 'approximation', 'gamma-point', 'Γ-point', 'zero-point energy', 'zpe']
for i, cell in enumerate(nb.get('cells', [])):
src = ''.join(cell.get('source', []))
low = src.lower()
if any(t.lower() in low for t in terms):
print(f"\n--- cell {i} ({cell.get('cell_type')}) ---")
print(src)
PYRepository: mat3ra/api-examples
Length of output: 3719
Call this a Γ-point approximation, not bulk ZPE. For periodic silicon, zero-point energy needs phonons across the Brillouin zone; this notebook only describes a Γ-only phonon step. Rename the result or switch to a converged q-point workflow.
🤖 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/zero_point_energy.ipynb` around lines 10 -
18, Update the zero-point energy notebook description to identify the result as
a Γ-point approximation rather than bulk or converged ZPE. Either rename result
labels and explanatory text accordingly, or replace the Γ-only phonon workflow
with a Brillouin-zone q-point workflow that computes converged bulk ZPE.
Comment on lines
+233
to
+235
| "projects = client.projects.list({\"isDefault\": True, \"owner._id\": ACCOUNT_ID})\n", | ||
| "project_id = projects[0][\"_id\"]\n", | ||
| "print(f\"✅ Using project: {projects[0]['name']} ({project_id})\")" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle accounts without a default project.
Line 234 raises IndexError when the selected personal or organization account has no default project. Validate the response and report an actionable configuration error.
Proposed fix
projects = client.projects.list({"isDefault": True, "owner._id": ACCOUNT_ID})
+if not projects:
+ raise RuntimeError(
+ f"No default project found for account {selected_account.name!r}"
+ )
project_id = projects[0]["_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
| "projects = client.projects.list({\"isDefault\": True, \"owner._id\": ACCOUNT_ID})\n", | |
| "project_id = projects[0][\"_id\"]\n", | |
| "print(f\"✅ Using project: {projects[0]['name']} ({project_id})\")" | |
| "projects = client.projects.list({\"isDefault\": True, \"owner._id\": ACCOUNT_ID})\n", | |
| "if not projects:\n", | |
| " raise RuntimeError(\n", | |
| " f\"No default project found for account {selected_account.name!r}\"\n", | |
| " )\n", | |
| "project_id = projects[0][\"_id\"]\n", | |
| "print(f\"✅ Using project: {projects[0]['name']} ({project_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 `@other/materials_designer/workflows/zero_point_energy.ipynb` around lines 233
- 235, Update the project selection flow around client.projects.list and
project_id so it checks whether the returned projects collection contains a
default project before indexing projects[0]. If none exists, raise or report an
actionable configuration error identifying the selected account and the need to
create or configure a default project; preserve the existing project_id
assignment and success message when a project is available.
Comment on lines
+498
to
+509
| "# 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
Validate cluster availability and name matching.
An empty cluster list causes clusters[0] to fail, while an unmatched CLUSTER_NAME leaves cluster=None and breaks compute construction or printing.
Proposed fix
+if not clusters:
+ raise RuntimeError("No compute clusters are available for this account")
+
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 matches {CLUSTER_NAME!r}")
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
| "# 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}\")" | |
| "# Select cluster: use specified name if provided, otherwise use first available\n", | |
| "if not clusters:\n", | |
| " raise RuntimeError(\"No compute clusters are available for this account\")\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(f\"No cluster matches {CLUSTER_NAME!r}\")\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/zero_point_energy.ipynb` around lines 498
- 509, Update the cluster selection logic before constructing Compute to
validate that clusters is non-empty and that a specified CLUSTER_NAME matches an
available cluster. Raise a clear error for either missing availability or an
unmatched name, and only pass a confirmed cluster to Compute and the subsequent
print statement.
timurbazhirov
approved these changes
Jul 14, 2026
Merged
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.
The NB was erroneously deleted upon merging from main and then back into it
Summary by CodeRabbit
New Features
Documentation