feature/SOF-7917 Feat: Defect formation energy - #346
Merged
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 Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a defect formation energy notebook, introduces source-scoped total-energy lookup, updates surface and interfacial workflows, removes notebook sanity-check output, links the new workflow, and refreshes Materials Project documentation and link-checking configuration. ChangesMaterials workflow updates
Repository link maintenance
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant DefectFormationEnergyNotebook
participant Mat3raAPI
participant ComputeCluster
User->>DefectFormationEnergyNotebook: configure defect and pristine inputs
DefectFormationEnergyNotebook->>Mat3raAPI: authenticate and resolve materials and references
DefectFormationEnergyNotebook->>Mat3raAPI: create and submit multi-material job
Mat3raAPI->>ComputeCluster: execute patched DFT workflow
ComputeCluster-->>Mat3raAPI: return completed job
DefectFormationEnergyNotebook->>Mat3raAPI: retrieve and visualize formation energy
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 4
🧹 Nitpick comments (1)
other/materials_designer/workflows/defect_formation_energy.ipynb (1)
376-389: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using a shared helper for material resolution.
If the newly added
analysis.pyutilities (mentioned in the PR stack) include a generic helper for resolving materials with a specific property by hash and name, consider using it here to replaceresolve_pristine_with_total_energyand reduce code duplication.🤖 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 376 - 389, Replace the local resolve_pristine_with_total_energy implementation with the shared analysis.py helper that resolves materials by hash and name while requiring a specific property. Pass the existing material, name, and Total Energy lookup requirements through that helper, preserving hash-first resolution, name fallback, duplicate avoidance, and the existing (Material, property) return contract.
🤖 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 120-123: Validate N_INTERFACES before applying the workflow,
ensuring it is explicitly provided and limited to the supported interface counts
for the structure. Update the apply_workflow_parameters path or its caller so
invalid or missing set-n-interfaces values fail clearly instead of leaving γ
normalization unset.
- Around line 245-254: Update the interface lookup in the `interface is None`
branch and the bulk hash resolver to include an `ACCOUNT_ID` filter in their
platform material queries. Preserve the existing name-regex and hash-matching
behavior while ensuring both lookups only return materials belonging to the
current account.
In `@other/materials_designer/workflows/surface_energy.ipynb`:
- Around line 292-297: Add the missing find_total_energy_for_material function
to mat3ra.notebooks_utils.core.entity.property.api, ensuring it is exported and
supports the call with client and bulk_material.id used by surface_energy.ipynb.
Reuse the existing total-energy property lookup behavior and return the expected
tuple so the bulk-energy lookup proceeds without ImportError.
In `@src/py/mat3ra/notebooks_utils/core/entity/material/analysis.py`:
- Around line 162-165: Update find_owned_material to include owner._id
constrained to owner_id in the bulk_query passed to api_client.materials.list,
then return only the matching owned material. Remove the fallback to matches[0]
so the function returns None when no material belongs to owner_id.
---
Nitpick comments:
In `@other/materials_designer/workflows/defect_formation_energy.ipynb`:
- Around line 376-389: Replace the local resolve_pristine_with_total_energy
implementation with the shared analysis.py helper that resolves materials by
hash and name while requiring a specific property. Pass the existing material,
name, and Total Energy lookup requirements through that helper, preserving
hash-first resolution, name fallback, duplicate avoidance, and the existing
(Material, property) return 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: a6512c90-5ff5-4af4-b6f9-58dda3d52e8c
📒 Files selected for processing (6)
README.mdother/materials_designer/workflows/Introduction.ipynbother/materials_designer/workflows/defect_formation_energy.ipynbother/materials_designer/workflows/interfacial_energy.ipynbother/materials_designer/workflows/surface_energy.ipynbsrc/py/mat3ra/notebooks_utils/core/entity/material/analysis.py
Comment on lines
+162
to
+165
| 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.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not fall back to another account’s material.
When no match belongs to owner_id, Line 165 returns the first global match. The notebook can consequently validate another account’s Total Energy while submitting under the selected account. Include owner._id in the API query and return None when no owned match exists.
Proposed fix
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)
+ matches = api_client.materials.list({**bulk_query, "owner._id": owner_id})
+ return matches[0] if matches else None📝 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
| 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) | |
| def find_owned_material(api_client: Any, bulk_query: dict, owner_id: str) -> Optional[Any]: | |
| matches = api_client.materials.list({**bulk_query, "owner._id": owner_id}) | |
| return matches[0] if matches else None |
🤖 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 to include owner._id constrained to
owner_id in the bulk_query passed to api_client.materials.list, then return only
the matching owned material. Remove the fallback to matches[0] so the function
returns None when no material belongs to owner_id.
VsevolodX
changed the title
timurbazhirov
approved these changes
Jul 21, 2026
Matches formation_energy.ipynb's own ELEMENTAL_TE_SOURCE default -- most callers want the best-precision total_energy regardless of owner, not scoped to the current account.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
other/materials_designer/workflows/defect_formation_energy.ipynb (1)
356-369: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestrict elemental references to Standata.
This query accepts any accessible material tagged
elemental, despite the notebook promising Standata references. A user-owned or third-party match can therefore alter the chemical potentials and formation energy. Filter by the Standata/curator owner identity before grouping results.🤖 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 356 - 369, Update the elemental_materials_data query to restrict results to the established Standata/curator owner identity before element_materials_by_symbol groups them, while preserving the existing elemental tag and metadata.element filters and missing-element validation.
🤖 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 137-140: Update the documented source options in
other/materials_designer/workflows/defect_formation_energy.ipynb lines 137-140,
other/materials_designer/workflows/interfacial_energy.ipynb lines 125-128, and
other/materials_designer/workflows/surface_energy.ipynb lines 114-117 so
“my_account” is described as the current account only; leave the other source
descriptions unchanged.
In `@src/py/mat3ra/notebooks_utils/core/entity/property/api.py`:
- Around line 96-102: Move the source validation in the property lookup flow
before the material’s exabyteId check or any fetch/inspection, so invalid values
such as "everyone" always raise ValueError even when exabyteId is missing.
Preserve the existing query construction for public, curators, and my_account,
and add a regression test covering an invalid source with a material lacking
exabyteId.
---
Outside diff comments:
In `@other/materials_designer/workflows/defect_formation_energy.ipynb`:
- Around line 356-369: Update the elemental_materials_data query to restrict
results to the established Standata/curator owner identity before
element_materials_by_symbol groups them, while preserving the existing elemental
tag and metadata.element filters and missing-element validation.
🪄 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: 460df256-ba76-4bbb-bdf8-bd0dfe325813
📒 Files selected for processing (11)
.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/interfacial_energy.ipynbother/materials_designer/workflows/phonon_dos_dispersion.ipynbother/materials_designer/workflows/surface_energy.ipynbsrc/py/mat3ra/notebooks_utils/core/entity/property/api.pytests/py/unit/core/entity/test_property_api_find_total_energy_for_material.py
Comment on lines
+137
to
+140
| "# Whose total_energy properties to consider for the pristine reference:\n", | ||
| "# \"public\" (any owner, highest precision wins), \"curators\" (only curators'),\n", | ||
| "# or \"my_account\" (curators' or your own).\n", | ||
| "PRISTINE_TOTAL_ENERGY_SOURCE = \"my_account\"\n" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the documented my_account scope.
find_total_energy_for_material(..., source="my_account") filters only by owner._id; it does not include curator properties. The current guidance can make users expect a curator fallback that will not occur.
other/materials_designer/workflows/defect_formation_energy.ipynb#L137-L140: describemy_accountas the current account only.other/materials_designer/workflows/interfacial_energy.ipynb#L125-L128: describemy_accountas the current account only.other/materials_designer/workflows/surface_energy.ipynb#L114-L117: describemy_accountas the current account only.
📍 Affects 3 files
other/materials_designer/workflows/defect_formation_energy.ipynb#L137-L140(this comment)other/materials_designer/workflows/interfacial_energy.ipynb#L125-L128other/materials_designer/workflows/surface_energy.ipynb#L114-L117
🤖 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 137 - 140, Update the documented source options in
other/materials_designer/workflows/defect_formation_energy.ipynb lines 137-140,
other/materials_designer/workflows/interfacial_energy.ipynb lines 125-128, and
other/materials_designer/workflows/surface_energy.ipynb lines 114-117 so
“my_account” is described as the current account only; leave the other source
descriptions unchanged.
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 | 🟡 Minor | ⚡ Quick win
Validate source before fetching or inspecting the material.
With a material lacking exabyteId, execution returns None at Line 95 before reaching this validation, so source="everyone" is silently accepted. Validate first and add a regression test for invalid source plus missing exabyteId.
Proposed fix
+ if source not in ("public", "curators", "my_account"):
+ raise ValueError(
+ f"Invalid source: {source!r}. Expected 'public', 'curators', or 'my_account'."
+ )
+
material = client.materials.get(material_id)
exabyte_id = material.get("exabyteId")
if not exabyte_id:
return None
@@
- elif source != "public":
- raise ValueError(f"Invalid source: {source!r}. Expected 'public', 'curators', or 'my_account'.")🤖 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, Move the source validation in the property lookup flow before the
material’s exabyteId check or any fetch/inspection, so invalid values such as
"everyone" always raise ValueError even when exabyteId is missing. Preserve the
existing query construction for public, curators, and my_account, and add a
regression test covering an invalid source with a material lacking exabyteId.
property/api.py imports mat3ra.api_client and mat3ra.prode at module level, but the tests extras group only pulled in [materials] + mat3ra-wode -- never [api], and mat3ra-prode isn't in [materials] either. No prior test imported this module, so a clean install (CI) never surfaced the gap; a locally pre-populated venv masked it. Use [workflows] instead, which already bundles materials + api + prode + wode + mode + ade + ide together.
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.
Summary by CodeRabbit