Feature/sof-7954 Feat: Interfacial Energy WF#344
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an interfacial-energy workflow notebook and links it from the materials-designer documentation. Refactors shared bulk resolution, adds reusable SCF k-grid application, and updates the surface-energy notebook to use existing bulk total-energy properties and a previously loaded workflow. ChangesEnergy workflow updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Notebook
participant Mat3raAPI
participant WorkflowStandata
participant ComputeJob
Notebook->>Mat3raAPI: authenticate and load interface and bulk materials
Notebook->>WorkflowStandata: load and configure Interfacial Energy workflow
Notebook->>ComputeJob: create and submit interfacial job
ComputeJob-->>Notebook: completed energy properties
Notebook->>Mat3raAPI: retrieve and visualize results
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
other/materials_designer/workflows/interfacial_energy.ipynb (1)
354-381: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated bulk-resolution logic (substrate vs. film).
This cell is nearly identical to the substrate cell (Lines 318-343), differing only by
part, theio-bulk-*unit name, and target variable names. The two TE-check cells (Lines 415-455 and 466-504) share the same duplication. Consider extracting a helper (e.g.,resolve_bulk(part, io_unit_name)andcheck_bulk_total_energy(subworkflow, ids, ...)) to keep the query/projection extraction in one place and reduce drift.🤖 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/interfacial_energy.ipynb` around lines 354 - 381, Extract the duplicated substrate/film bulk-resolution flow into a shared helper such as resolve_bulk(part, io_unit_name), covering interface crystal lookup, query/projection extraction, material lookup, missing-result validation, and response conversion. Update both bulk-resolution cells to call it with their respective identifiers, and similarly consolidate the duplicated total-energy checks through a shared check_bulk_total_energy helper while preserving existing outputs and error behavior.other/materials_designer/workflows/surface_energy.ipynb (1)
295-322: 🔒 Security & Privacy | 🔵 Trivial | ⚖️ Poor tradeoff
eval()on workflow-derived template strings is a fragile/insecure pattern.Multiple
eval(..., {"__builtins__": {}}, ...)calls (lines 301, 306, 309, 311, 322) evaluate template strings pulled from workflow subworkflow/unit definitions. Clearing__builtins__is a commonly-bypassed mitigation (e.g. via object-graph traversal like().__class__.__base__.__subclasses__()), not a real sandbox — flagged by Ruff as S307. Since these templates originate from platform-authored workflow definitions rather than end-user input, the immediate exploitability is lower, but this pattern still deserves scrutiny given it's now used repeatedly across the notebook (also at lines 404-405) as the sole templating mechanism for extracting query/projection data.Consider having the workflow templates expose structured (non-eval'd) accessors, or restrict evaluation to a minimal safe-expression parser (e.g.
ast.literal_evalwon't work here since the templates reference variables likeSLAB/BULK_ID, so a small allow-listed AST-based evaluator would be needed instead).🤖 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/surface_energy.ipynb` around lines 295 - 322, Replace the eval-based template handling in the workflow extraction block, including the related calls around the later query/projection extraction, with structured workflow accessors or a minimal allow-listed AST evaluator. Preserve variable substitution for SLAB, BULK_ID, and LAST_BUILD_CRYSTAL while allowing only the required expression forms and rejecting all other nodes; do not rely on an empty __builtins__ dictionary as sandboxing.
🤖 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/surface_energy.ipynb`:
- Around line 313-317: Update the ValueError message in the bulk_query
validation to accurately cover both ID-based and crystal-build-based lookup
failures, rather than asserting missing crystal scaledHash or hash. Preserve the
guidance to upload the bulk material and run total_energy.ipynb first.
- Around line 270-277: Replace the positional subworkflow lookups in the
surface_workflow setup with resolution by each subworkflow’s stable identifier,
or use a single helper that defines and validates the ordering contract. Ensure
resolve_bulk_by_id_subworkflow, resolve_bulk_by_build_subworkflow, and
get_bulk_energy_subworkflow remain bound to the correct named subworkflows if
the template order changes.
In `@src/py/mat3ra/notebooks_utils/core/entity/material/analysis.py`:
- Around line 162-165: Update find_owned_material so it does not return an
unowned matches[0] as though it were an owned match; return an ownership
indicator or None for the fallback, and adjust get_bulk_material to distinguish
and log the owned-match and non-owned fallback paths correctly.
---
Nitpick comments:
In `@other/materials_designer/workflows/interfacial_energy.ipynb`:
- Around line 354-381: Extract the duplicated substrate/film bulk-resolution
flow into a shared helper such as resolve_bulk(part, io_unit_name), covering
interface crystal lookup, query/projection extraction, material lookup,
missing-result validation, and response conversion. Update both bulk-resolution
cells to call it with their respective identifiers, and similarly consolidate
the duplicated total-energy checks through a shared check_bulk_total_energy
helper while preserving existing outputs and error behavior.
In `@other/materials_designer/workflows/surface_energy.ipynb`:
- Around line 295-322: Replace the eval-based template handling in the workflow
extraction block, including the related calls around the later query/projection
extraction, with structured workflow accessors or a minimal allow-listed AST
evaluator. Preserve variable substitution for SLAB, BULK_ID, and
LAST_BUILD_CRYSTAL while allowing only the required expression forms and
rejecting all other nodes; do not rely on an empty __builtins__ dictionary as
sandboxing.
🪄 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: bae0442b-9df5-4727-ab15-d026481480c7
📒 Files selected for processing (5)
README.mdother/materials_designer/workflows/Introduction.ipynbother/materials_designer/workflows/interfacial_energy.ipynbother/materials_designer/workflows/surface_energy.ipynbsrc/py/mat3ra/notebooks_utils/core/entity/material/analysis.py
| def find_owned_material(api_client: Any, bulk_query: dict, owner_id: str) -> Optional[Any]: | ||
| matches = api_client.materials.list(bulk_query) | ||
| owned = next((item for item in matches if item.get("owner", {}).get("_id") == owner_id), None) | ||
| return owned or (matches[0] if matches else None) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- analysis.py around find_owned_material and get_bulk_material ---'
nl -ba src/py/mat3ra/notebooks_utils/core/entity/material/analysis.py | sed -n '120,210p'
echo
echo '--- usages of find_owned_material / get_bulk_material ---'
rg -n "find_owned_material|get_bulk_material" src/py -S
echo
echo '--- broader context for bulk/material ownership logic ---'
rg -n "owner_id|owner.*_id|owned material|bulk material|Found exact bulk material|bulk_query" src/py/mat3ra/notebooks_utils -SRepository: mat3ra/api-examples
Length of output: 266
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- analysis.py around find_owned_material and get_bulk_material ---'
sed -n '120,210p' src/py/mat3ra/notebooks_utils/core/entity/material/analysis.py
echo
echo '--- usages of find_owned_material / get_bulk_material ---'
rg -n "find_owned_material|get_bulk_material" src/py -S
echo
echo '--- broader context for bulk/material ownership logic ---'
rg -n "owner_id|owner.*_id|owned material|bulk material|Found exact bulk material|bulk_query" src/py/mat3ra/notebooks_utils -SRepository: mat3ra/api-examples
Length of output: 6065
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- call sites of get_bulk_material ---'
rg -n "get_bulk_material\(" src/py -S
echo
echo '--- surrounding context at each call site ---'
python3 - <<'PY'
from pathlib import Path
import re
path = Path("src/py/mat3ra/notebooks_utils/core/entity/material/analysis.py")
text = path.read_text().splitlines()
for i, line in enumerate(text, 1):
if "get_bulk_material(" in line and "def " not in line:
start = max(1, i-15)
end = min(len(text), i+25)
print(f"\n### {path}:{i}\n")
for j in range(start, end+1):
print(f"{j:4d}: {text[j-1]}")
PYRepository: mat3ra/api-examples
Length of output: 391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- docs/comments mentioning shared/public bulk or material ownership ---'
rg -n "public|shared|owner|ownership|bulk material|bulkId|hash" src/py/mat3ra/notebooks_utils -S
echo
echo '--- material API helpers for owner-aware lookups ---'
sed -n '1,120p' src/py/mat3ra/notebooks_utils/core/entity/material/api.py
echo
echo '--- workflow API helper for comparison ---'
sed -n '1,120p' src/py/mat3ra/notebooks_utils/core/entity/workflow/api.pyRepository: mat3ra/api-examples
Length of output: 7855
Handle the non-owned fallback explicitly. find_owned_material returns matches[0] when no owned match exists, so get_bulk_material can log Found exact bulk material for a material owned by someone else. Return an ownership flag or None here so the caller can distinguish the fallback 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/analysis.py` around lines
162 - 165, Update find_owned_material so it does not return an unowned
matches[0] as though it were an owned match; return an ownership indicator or
None for the fallback, and adjust get_bulk_material to distinguish and log the
owned-match and non-owned fallback paths correctly.
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 `@other/materials_designer/workflows/interfacial_energy.ipynb`:
- Around line 118-123: Validate N_INTERFACES before it is used to configure the
workflow, accepting only the documented integer values 1 or 2 and rejecting
zero, negative, fractional, or other values with a clear error. Preserve the
existing interface-count semantics for valid configurations.
- Around line 346-383: The bulk lookup in resolve_bulk_material must be
restricted to ACCOUNT_ID so it cannot select another account’s hash twin or
energy property. Reuse get_bulk_material with ACCOUNT_ID if available, or
constrain the materials.list query with owner._id equal to ACCOUNT_ID, while
preserving the existing selection of a candidate with finished Total Energy and
ensuring the returned bulk and property belong to that owner.
🪄 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: ab6c5d67-6e42-4e9d-8afe-704d92d71394
📒 Files selected for processing (3)
other/materials_designer/workflows/Introduction.ipynbother/materials_designer/workflows/interfacial_energy.ipynbother/materials_designer/workflows/surface_energy.ipynb
| @@ -0,0 +1,683 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #18. def find_bulk_with_total_energy(crystal, role, material_index):
These two should be reused across sufrace energy and interfacial energy nbs
Reply via ReviewNB
| @@ -0,0 +1,683 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #5. def apply_workflow_parameters(workflow: Workflow, scf_kgrid=None, n_interfaces=None) -> Workflow:
Is this reusable too?
Reply via ReviewNB
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
other/materials_designer/workflows/interfacial_energy.ipynb (1)
586-607: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
interface_te_datais fetched but never used.Line 596-598 fetches the interface's total-energy property via an API call, but it's never passed to
visualize_propertiesor referenced in the print statements below (only substrate/film energies are printed). This is either dead code (extra unnecessary API call) or an incomplete feature — the interface's own total energy should likely be surfaced alongside substrate/film energies for consistency.♻️ Suggested fix: surface the fetched value
print(f"Interface (material 0): {saved_interface.name} ({saved_interface.id})") +print(f" total energy: {interface_te_data['data']['value']} eV") print(f"Substrate bulk (material 1): {substrate_bulk.name} ({substrate_bulk.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/interfacial_energy.ipynb` around lines 586 - 607, Use the fetched interface_te_data in the interfacial energy workflow instead of leaving it unused: surface the interface total-energy value alongside the substrate and film energies in the output, reusing the existing interface identity and value-access patterns.
🤖 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 `@tests/py/unit/test_workflow_utils.py`:
- Around line 52-60: Update test_apply_scf_kgrid_updates_pw_scf_context to
locate the context item named "kgrid" and assert that its "dimensions" value
equals SCF_KGRID directly. Remove the broad predicate that accepts any item
containing a "dimensions" key.
---
Nitpick comments:
In `@other/materials_designer/workflows/interfacial_energy.ipynb`:
- Around line 586-607: Use the fetched interface_te_data in the interfacial
energy workflow instead of leaving it unused: surface the interface total-energy
value alongside the substrate and film energies in the output, reusing the
existing interface identity and value-access patterns.
🪄 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: 5e317c77-9dc7-4e7b-84fb-d71486adcb5e
📒 Files selected for processing (4)
other/materials_designer/workflows/interfacial_energy.ipynbother/materials_designer/workflows/surface_energy.ipynbsrc/py/mat3ra/notebooks_utils/workflow.pytests/py/unit/test_workflow_utils.py
| return workflow | ||
| context = PointsGridDataProvider(dimensions=scf_kgrid, isEdited=True).get_context_item_data() | ||
| for subworkflow in workflow.subworkflows: | ||
| if "pw_scf" not in [unit.name for unit in subworkflow.units]: |
| return np.linalg.norm(crossprod) | ||
|
|
||
|
|
||
| def get_slab_bulk_crystal(slab_material: Material) -> dict: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/py/unit/core/entity/test_material_analysis.py`:
- Line 7: Update the shared SILICON fixture and its uses in
resolve_bulk_query_from_crystal tests to copy the material data and remove
scaledHash, hash, and _id before merging extra_keys. Apply this sanitized
fixture consistently, including
test_resolve_bulk_query_computes_hash_when_none_present, so hash-only, _id-only,
and computed-hash fallback paths are exercised.
🪄 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: 7bab1722-8574-4992-a39c-d80f1f14424a
📒 Files selected for processing (8)
other/materials_designer/workflows/interfacial_energy.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
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/py/unit/test_workflow_utils.py
- other/materials_designer/workflows/interfacial_energy.ipynb
| from mat3ra.notebooks_utils.core.entity.material.analysis import resolve_bulk_query_from_crystal | ||
| from mat3ra.standata.materials import Materials | ||
|
|
||
| SILICON = Materials.get_by_name_first_match("Silicon") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
from mat3ra.standata.materials import Materials
material = Materials.get_by_name_first_match("Silicon")
print({key: material.get(key) for key in ("scaledHash", "hash", "_id")})
PYRepository: mat3ra/api-examples
Length of output: 276
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file list candidate =="
git ls-files | rg 'test_material_analysis|materials|Material|materials' | head -200
echo "== test file =="
if [ -f tests/py/unit/core/entity/test_material_analysis.py ]; then
cat -n tests/py/unit/core/entity/test_material_analysis.py
fi
echo "== resolve_bulk_query definitions =="
rg -n "resolve_bulk_query_from_crystal|class Materials|def get_by_name_first_match|scaledHash|hash|_id" -S . | head -300Repository: mat3ra/api-examples
Length of output: 49602
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== resolution implementation =="
cat -n src/py/mat3ra/notebooks_utils/core/entity/material/analysis.py | sed -n '100,135p'
echo "== Materials source candidates =="
git ls-files '*materials*' | sed -n '1,120p'
rg -n "class Materials|def get_by_name_first_match|get_by_name_first_match|standata|materials" src tests other examples | sed -n '1,200p'Repository: mat3ra/api-examples
Length of output: 41207
🌐 Web query:
mat3ra standata Materials Silicon get_by_name_first_match scaledHash hash _id
💡 Result:
In the context of the Mat3ra platform and its data standards, the terms provided relate to material structure identification and data retrieval within the platform's API and schema [1][2][3][4]. 1. scaledHash: This is a property of a material entity, represented as a string [3][4]. It acts as a hash used to identify structures that are identical even under different uniform pressure [3][4]. It is calculated by scaling all dimensions of the material's primitive unit cell by the lattice constant 'a' so that 'a' is set to 1.0 [3][4]. 2. hash: Often used alongside scaledHash to uniquely identify materials or structural configurations [4][5]. 3. _id: This typically refers to the unique identifier for an entity (such as a material, owner, or subworkflow) within the Mat3ra platform's database [4][5]. 4. get_by_name_first_match: While not a documented top-level API function for the Materials entity, similar naming patterns (e.g., resolve_bulk_material, get_bulk_material) are used in the platform's API-examples repository to resolve material or subworkflow entities by criteria such as identifiers, hashes, or names [5]. The ESSE (Essential Source of Schemas and Examples) Data Convention serves as the foundation for these structures, using JSON to organize entity properties [2][4]. For example, a material sample of Silicon would be represented with these properties to ensure consistent identification across the platform [1][4].
Citations:
- 1: https://docs.mat3ra.com/reference/data-structured/convention/
- 2: https://docs.mat3ra.com/standards/data-structured/convention/
- 3: https://docs.mat3ra.com/materials/data
- 4: https://docs.mat3ra.com/standards/materials/data/
- 5: Feature/sof-7954 Feat: Interfacial Energy WF #344
🌐 Web query:
site:docs.mat3ra.com/materials/data Silicon scaledHash hash _id
💡 Result:
For the Silicon FCC example in Mat3ra docs, the material JSON shows:
_id:LCthJ6E2QabYCZqf3hash:4ac3fa057e3530628eb1295c77110eaascaledHash:5bd4eb168e3530628eb1295d88221cbb(docs.mat3ra.com)
The docs define hash as the structure-based hash for the primitive standard representation, and scaledHash as the same idea after scaling the primitive cell so lattice vector a = 1.0, to identify the same structure under uniform pressure. (docs.mat3ra.com)
Citations:
- 1: https://docs.mat3ra.com/materials/data/?utm_source=openai
- 2: https://docs.mat3ra.com/materials/data/?utm_source=openai
Remove resolver identifiers from the shared Silicon fixture.
SILICON includes scaledHash, hash, and _id, and resolve_bulk_query_from_crystal() returns whichever of those fields is present. This masks the hash-only, _id-only, and computed-hash fallback paths; use a copy with those three keys removed before merging extra_keys, including the test_resolve_bulk_query_computes_hash_when_none_present case.
🤖 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 `@tests/py/unit/core/entity/test_material_analysis.py` at line 7, Update the
shared SILICON fixture and its uses in resolve_bulk_query_from_crystal tests to
copy the material data and remove scaledHash, hash, and _id before merging
extra_keys. Apply this sanitized fixture consistently, including
test_resolve_bulk_query_computes_hash_when_none_present, so hash-only, _id-only,
and computed-hash fallback paths are exercised.
2 participants
Summary by CodeRabbit