feat: HOMO/LUMO NB#351
Open
VsevolodX wants to merge 14 commits into
Open
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
📝 WalkthroughWalkthroughAdds defect formation energy, interfacial energy, and HOMO–LUMO/frequency notebooks; introduces shared bulk-material, total-energy, and SCF k-grid utilities; updates surface-energy integration, notebook outputs, documentation links, and link checking. ChangesMaterials Designer workflow expansion
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Notebook
participant MaterialsAPI
participant WorkflowAPI
participant JobAPI
participant PropertiesAPI
Notebook->>MaterialsAPI: prepare materials and references
Notebook->>WorkflowAPI: configure workflow
Notebook->>JobAPI: create and submit calculation
JobAPI-->>Notebook: completed job
Notebook->>PropertiesAPI: retrieve calculated properties
PropertiesAPI-->>Notebook: visualization data
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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)
other/materials_designer/workflows/defect_formation_energy.ipynb (1)
458-465: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
apply_scf_kgridinstead of reimplementing the k-grid patch.This block duplicates the shared
apply_scf_kgridhelper that the surface and interfacial notebooks already call. Reusing it keeps behavior consistent (e.g., thefirst_onlyoption and unit-name handling) and avoids drift.♻️ Suggested change
-# K-grid for the defective-cell SCF. -if SCF_KGRID is not None: - new_context = PointsGridDataProvider(dimensions=SCF_KGRID, isEdited=True).get_context_item_data() - for subworkflow in defect_workflow.subworkflows: - unit = subworkflow.get_unit_by_name(name="pw_scf") - if unit: - unit.add_context(new_context) - subworkflow.set_unit(unit) +# K-grid for the defective-cell SCF. +defect_workflow = apply_scf_kgrid(defect_workflow, scf_kgrid=SCF_KGRID)This also lets you drop the
PointsGridDataProviderimport and addapply_scf_kgridto the imports in this cell.🤖 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/defect_formation_energy.ipynb` around lines 458 - 465, Replace the inline SCF k-grid patching block with the shared apply_scf_kgrid helper, preserving the defective workflow and SCF_KGRID inputs. Update the cell imports to add apply_scf_kgrid and remove the now-unused PointsGridDataProvider import.
🤖 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/defect_formation_energy.ipynb`:
- Around line 366-369: Update the RuntimeError message in the defect formation
energy workflow to remove the non-interpolated `{element}` placeholder and
reword the guidance generically for the missing elemental materials, while
preserving the existing missing-material details.
In `@other/materials_designer/workflows/homo_lumo_frequency.ipynb`:
- Around line 514-530: Guard the HOMO/LUMO gap calculation in the
CALCULATE_HOMO_LUMO block after fetching homo_data and lumo_data, checking that
both results are non-empty before indexing their first values. When either
result is empty, emit a clear message and skip the subtraction and formatted gap
output; preserve visualization for any returned data.
- Around line 296-312: Validate the standata lookups and required structure
before composing the workflow in the notebook: ensure the selected base entry
exists and contains the expected HOMO/LUMO configuration, and when both
CALCULATE_HOMO_LUMO and CALCULATE_FREQUENCY are enabled, ensure frequency.json
exists with a valid subworkflows[0] entry. Use specific lookup behavior and
raise clear, actionable errors for missing or malformed entries instead of
allowing KeyError/IndexError or silently omitting frequency calculations;
preserve the existing workflow composition for valid data.
In `@README.md`:
- Line 163: Synchronize the Materials Project API-key guidance by updating the
legacy URL references in README.ipynb and the settings configuration near the
API-key definition to the next-gen Materials Project URL used in README.md. Keep
the guidance consistent across all three locations unless a documented
compatibility reason requires retaining the legacy URL.
In `@src/py/mat3ra/notebooks_utils/core/entity/property/api.py`:
- Around line 96-102: Update find_total_energy_for_material so
source="my_account" filters by the resolved owner account used to create or
resolve the material, rather than always using client.my_account.id. Thread that
account through its callers, including organization accounts selected via
ACCOUNT_ID, while preserving curator ownership handling when the documented
my_account source includes curators.
---
Nitpick comments:
In `@other/materials_designer/workflows/defect_formation_energy.ipynb`:
- Around line 458-465: Replace the inline SCF k-grid patching block with the
shared apply_scf_kgrid helper, preserving the defective workflow and SCF_KGRID
inputs. Update the cell imports to add apply_scf_kgrid and remove the now-unused
PointsGridDataProvider import.
🪄 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: 5626a0b9-5a13-49d9-903f-cfdec2fad19d
📒 Files selected for processing (16)
.github/workflows/check_links.ymlREADME.mdexamples/assets/README.mdother/materials_designer/workflows/Introduction.ipynbother/materials_designer/workflows/defect_formation_energy.ipynbother/materials_designer/workflows/dielectric_tensor.ipynbother/materials_designer/workflows/homo_lumo_frequency.ipynbother/materials_designer/workflows/interfacial_energy.ipynbother/materials_designer/workflows/phonon_dos_dispersion.ipynbother/materials_designer/workflows/surface_energy.ipynbsrc/py/mat3ra/notebooks_utils/core/entity/material/analysis.pysrc/py/mat3ra/notebooks_utils/core/entity/material/api.pysrc/py/mat3ra/notebooks_utils/core/entity/property/api.pysrc/py/mat3ra/notebooks_utils/workflow.pytests/py/unit/core/entity/test_material_analysis.pytests/py/unit/test_workflow_utils.py
Comment on lines
+366
to
+369
| " raise RuntimeError(\n", | ||
| " f\"Missing elemental reference material(s) for {missing}. \"\n", | ||
| " \"Add elemental material(s) from Standata, or add tag 'elemental' with metadata.element = {element}\"\n", | ||
| " )\n", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
{element} won't interpolate in this error message.
Only the first concatenated string is an f-string; the second is a plain literal, so {element} is printed verbatim (and element isn't in scope here anyway). Reword to avoid the misleading placeholder.
🐛 Suggested fix
raise RuntimeError(
f"Missing elemental reference material(s) for {missing}. "
- "Add elemental material(s) from Standata, or add tag 'elemental' with metadata.element = {element}"
+ "Add the elemental material(s) from Standata, or tag existing ones 'elemental' "
+ "with a matching metadata.element."
)📝 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
| " raise RuntimeError(\n", | |
| " f\"Missing elemental reference material(s) for {missing}. \"\n", | |
| " \"Add elemental material(s) from Standata, or add tag 'elemental' with metadata.element = {element}\"\n", | |
| " )\n", | |
| " raise RuntimeError(\n", | |
| " f\"Missing elemental reference material(s) for {missing}. \"\n", | |
| " \"Add the elemental material(s) from Standata, or tag existing ones 'elemental' \"\n", | |
| " \"with a matching metadata.element.\"\n", | |
| " )\n", |
🤖 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/defect_formation_energy.ipynb` around
lines 366 - 369, Update the RuntimeError message in the defect formation energy
workflow to remove the non-interpolated `{element}` placeholder and reword the
guidance generically for the missing elemental materials, while preserving the
existing missing-material details.
Comment on lines
+296
to
+312
| "from mat3ra.standata.workflows import WorkflowStandata\n", | ||
| "from mat3ra.wode import Workflow, Subworkflow\n", | ||
| "from mat3ra.notebooks_utils.ipython.entity.workflow.visualize import visualize_workflow\n", | ||
| "\n", | ||
| "nwchem_workflows = WorkflowStandata.filter_by_application(app.name)\n", | ||
| "\n", | ||
| "base_search_term = \"total_energy.json\" if CALCULATE_HOMO_LUMO else \"frequency.json\"\n", | ||
| "workflow = Workflow.create(nwchem_workflows.get_by_name_first_match(base_search_term))\n", | ||
| "\n", | ||
| "if CALCULATE_HOMO_LUMO and CALCULATE_FREQUENCY:\n", | ||
| " frequency_config = nwchem_workflows.get_by_name_first_match(\"frequency.json\")\n", | ||
| " workflow.add_subworkflow(Subworkflow(**frequency_config[\"subworkflows\"][0]))\n", | ||
| "\n", | ||
| "workflow.name = MY_WORKFLOW_NAME\n", | ||
| "\n", | ||
| "visualize_workflow(workflow)\n" | ||
| ] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the NWChem workflow standata to confirm structure/property outputs
fd -e json total_energy frequency -x cat -n {}Repository: mat3ra/api-examples
Length of output: 254
🏁 Script executed:
#!/bin/bash
set -u
echo "== files matching homo/freq/workflow names =="
git ls-files | rg -n 'homo_lumo|frequency|total_energy|standata|workflow' || true
echo
echo "== target cell context =="
sed -n '260,330p' other/materials_designer/workflows/homo_lumo_frequency.ipynb 2>/dev/null || true
echo
echo "== find JSON files with total_energy or frequency =="
git ls-files | rg -n '(^|/)(total_energy|frequency)\.json$|standata|workflows' || trueRepository: mat3ra/api-examples
Length of output: 6136
Guard the standata workflow composition assumptions.
WorkflowStandata JSON entries are external to this repo, so this notebook assumes total_energy.json has the HOMO/LUMO properties/values and frequency.json["subworkflows"][0] exists. Add fail-safes or explicit checks and use more specific lookup behavior, otherwise the workflow can fail with a confusing KeyError/IndexError or silently skip the requested frequencies.
🤖 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/homo_lumo_frequency.ipynb` around lines
296 - 312, Validate the standata lookups and required structure before composing
the workflow in the notebook: ensure the selected base entry exists and contains
the expected HOMO/LUMO configuration, and when both CALCULATE_HOMO_LUMO and
CALCULATE_FREQUENCY are enabled, ensure frequency.json exists with a valid
subworkflows[0] entry. Use specific lookup behavior and raise clear, actionable
errors for missing or malformed entries instead of allowing KeyError/IndexError
or silently omitting frequency calculations; preserve the existing workflow
composition for valid data.
Comment on lines
+514
to
+530
| { | ||
| "cell_type": "code", | ||
| "execution_count": null, | ||
| "id": "40", | ||
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "from mat3ra.notebooks_utils.ipython.entity.property.visualize import visualize_properties\n", | ||
| "\n", | ||
| "if CALCULATE_HOMO_LUMO:\n", | ||
| " homo_data = client.properties.get_for_job(job_id, property_name=\"homo_energy\")\n", | ||
| " lumo_data = client.properties.get_for_job(job_id, property_name=\"lumo_energy\")\n", | ||
| " visualize_properties(homo_data, title=\"HOMO Energy\")\n", | ||
| " visualize_properties(lumo_data, title=\"LUMO Energy\")\n", | ||
| " homo_lumo_gap = lumo_data[0][\"value\"] - homo_data[0][\"value\"]\n", | ||
| " print(f\"HOMO-LUMO gap: {homo_lumo_gap:.4f} eV\")\n" | ||
| ] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against empty HOMO/LUMO property results before indexing.
homo_data[0]["value"] / lumo_data[0]["value"] will raise an unguarded IndexError if get_for_job returns an empty list (e.g., the property wasn't produced for this job). Since this runs unconditionally right after fetching the data, a partial failure here crashes the notebook with a low-signal traceback instead of a clear message.
🛡️ Proposed fix
if CALCULATE_HOMO_LUMO:
homo_data = client.properties.get_for_job(job_id, property_name="homo_energy")
lumo_data = client.properties.get_for_job(job_id, property_name="lumo_energy")
visualize_properties(homo_data, title="HOMO Energy")
visualize_properties(lumo_data, title="LUMO Energy")
- homo_lumo_gap = lumo_data[0]["value"] - homo_data[0]["value"]
- print(f"HOMO-LUMO gap: {homo_lumo_gap:.4f} eV")
+ if homo_data and lumo_data:
+ homo_lumo_gap = lumo_data[0]["value"] - homo_data[0]["value"]
+ print(f"HOMO-LUMO gap: {homo_lumo_gap:.4f} eV")
+ else:
+ print("⚠️ HOMO/LUMO data not available for this job")📝 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": "40", | |
| "metadata": {}, | |
| "outputs": [], | |
| "source": [ | |
| "from mat3ra.notebooks_utils.ipython.entity.property.visualize import visualize_properties\n", | |
| "\n", | |
| "if CALCULATE_HOMO_LUMO:\n", | |
| " homo_data = client.properties.get_for_job(job_id, property_name=\"homo_energy\")\n", | |
| " lumo_data = client.properties.get_for_job(job_id, property_name=\"lumo_energy\")\n", | |
| " visualize_properties(homo_data, title=\"HOMO Energy\")\n", | |
| " visualize_properties(lumo_data, title=\"LUMO Energy\")\n", | |
| " homo_lumo_gap = lumo_data[0][\"value\"] - homo_data[0][\"value\"]\n", | |
| " print(f\"HOMO-LUMO gap: {homo_lumo_gap:.4f} eV\")\n" | |
| ] | |
| { | |
| "cell_type": "code", | |
| "execution_count": null, | |
| "id": "40", | |
| "metadata": {}, | |
| "outputs": [], | |
| "source": [ | |
| "from mat3ra.notebooks_utils.ipython.entity.property.visualize import visualize_properties\n", | |
| "\n", | |
| "if CALCULATE_HOMO_LUMO:\n", | |
| " homo_data = client.properties.get_for_job(job_id, property_name=\"homo_energy\")\n", | |
| " lumo_data = client.properties.get_for_job(job_id, property_name=\"lumo_energy\")\n", | |
| " visualize_properties(homo_data, title=\"HOMO Energy\")\n", | |
| " visualize_properties(lumo_data, title=\"LUMO Energy\")\n", | |
| " if homo_data and lumo_data:\n", | |
| " homo_lumo_gap = lumo_data[0][\"value\"] - homo_data[0][\"value\"]\n", | |
| " print(f\"HOMO-LUMO gap: {homo_lumo_gap:.4f} eV\")\n", | |
| " else:\n", | |
| " print(\"⚠️ HOMO/LUMO data not available for this job\")\n" | |
| ] |
🤖 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/homo_lumo_frequency.ipynb` around lines
514 - 530, Guard the HOMO/LUMO gap calculation in the CALCULATE_HOMO_LUMO block
after fetching homo_data and lumo_data, checking that both results are non-empty
before indexing their first values. When either result is empty, emit a clear
message and skip the subtraction and formatted gap output; preserve
visualization for any returned data.
| 3. Optionally, for local Jupyter without OIDC, set legacy API token values in [settings.json](src/py/mat3ra/notebooks_utils/core/api/settings.json). See [Get Authentication Params](examples/system/get_authentication_params.ipynb) for details. API tokens can also be generated in [Account Preferences](https://docs.mat3ra.com/accounts/ui/preferences/api/) on the platform. | ||
|
|
||
| NOTE: The Materials Project API key should be set in `settings.json` and obtained from [https://legacy.materialsproject.org/open](https://legacy.materialsproject.org/open). | ||
| NOTE: The Materials Project API key should be set in `settings.json` and obtained from [https://next-gen.materialsproject.org/api](https://next-gen.materialsproject.org/api). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Synchronize the remaining Materials Project API-key references.
README.ipynb (Lines 83-91) and src/py/mat3ra/notebooks_utils/core/api/settings.py (Lines 9-18) still point to the legacy URL, so repository guidance is now inconsistent. Update those references or explicitly document why they must remain legacy.
🤖 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 `@README.md` at line 163, Synchronize the Materials Project API-key guidance by
updating the legacy URL references in README.ipynb and the settings
configuration near the API-key definition to the next-gen Materials Project URL
used in README.md. Keep the guidance consistent across all three locations
unless a documented compatibility reason requires retaining the legacy URL.
Comment on lines
+96
to
+102
| query = {"exabyteId": exabyte_id, "slug": "total_energy"} | ||
| if source == "curators": | ||
| query["owner.slug"] = "curators" | ||
| elif source == "my_account": | ||
| query["owner._id"] = client.my_account.id | ||
| elif source != "public": | ||
| raise ValueError(f"Invalid source: {source!r}. Expected 'public', 'curators', or 'my_account'.") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm callers pass ACCOUNT_ID (possibly org) while this fn uses my_account.id
rg -nP --type=py -C2 'find_total_energy_for_material\s*\('
rg -nP --glob '*.ipynb' -C2 'find_total_energy_for_material'
# Inspect the my_account attribute source
rg -nP --type=py -C3 'my_account' src/py/mat3ra/notebooks_utilsRepository: mat3ra/api-examples
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -u
echo "Repo files matching api.py / notebooks_utils:"
git ls-files | grep -E '(^|/)api\.py$|notebooks_utils|\.ipynb$' | sed -n '1,200p'
echo
echo "Find target file:"
fd -ap 'api.py' . | sed -n '1,120p'
echo
echo "Search exact function name all files:"
rg -n -C3 'find_total_energy_for_material' . || true
echo
echo "Search total_energy related:"
rg -n -C2 'total_energy|TotalEnergy|Total Energy|TOTAL' . | sed -n '1,240p' || trueRepository: mat3ra/api-examples
Length of output: 38750
🏁 Script executed:
#!/bin/bash
set -u
echo "Target function:"
sed -n '60,130p' src/py/mat3ra/notebooks_utils/core/entity/property/api.py
echo
echo "Auth/client/account-related definitions and selected account setup:"
rg -n -C3 'selected_account|client =|APIClient|ACCOUNT_ID|ORGANIZATION_NAME|my_account|owner_id' src/py/mat3ra/notebooks_utils core src/py examples other -g '*.py' -g '*.ipynb' | sed -n '1,260p'
echo
echo "Bulk material functions:"
rg -n -C3 'def get_bulk_material|def get_bulk_material_by_crystal' src/py/mat3ra/notebooks_utils
sed -n '1,140p' src/py/mat3ra/notebooks_utils/core/entity/material/api.pyRepository: mat3ra/api-examples
Length of output: 28047
🏁 Script executed:
#!/bin/bash
set -u
python3 - <<'PY'
import json
from pathlib import Path
targets = [
"other/materials_designer/workflows/interfacial_energy.ipynb",
"other/materials_designer/workflows/surface_energy.ipynb",
"other/materials_designer/workflows/defect_formation_energy.ipynb",
"other/materials_designer/workflows/total_energy.ipynb",
]
for path in targets:
p = Path(path)
print(f"\n--- {path} ---")
data = json.loads(p.read_text())
cells = data.get("cells", [])
for i, cell in enumerate(cells):
source = "".join(cell.get("source", []))
if "ACCOUNT_ID" in source or "BULK_TOTAL_ENERGY_SOURCE" in source or "PRISTINE_TOTAL_ENERGY_SOURCE" in source:
print(f"[cell {i}]")
print(source)
PYRepository: mat3ra/api-examples
Length of output: 14219
Route source="my_account" through the resolved account selector.
The notebooks resolve and create materials under ACCOUNT_ID, which can be an organization account when ORGANIZATION_NAME is set, but find_total_energy_for_material(..., source="my_account") queries only client.my_account.id. This makes total-energy lookups fail even though the matching bulk/material was resolved under a different owner, breaking surface, interfacial, and defect formation energy notebooks. Thread the requested owner account into this lookup, including curators when the source documentation says my_account is “curators' or your own”.
🤖 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/property/api.py` around lines 96 -
102, Update find_total_energy_for_material so source="my_account" filters by the
resolved owner account used to create or resolve the material, rather than
always using client.my_account.id. Thread that account through its callers,
including organization accounts selected via ACCOUNT_ID, while preserving
curator ownership handling when the documented my_account source includes
curators.
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.
Summary by CodeRabbit