feature/SOF-7947 ZPE NB#341
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds three new Jupyter notebooks for materials-designer workflows (solid solution creation, convex hull/phase stability analysis, and zero-point energy DFT phonon workflow), two new Python modules for phase stability analysis and convex hull plotting, and updates link references in the Introduction notebook. ChangesSolid Solution Notebook
Phase Stability / Convex Hull Analysis
Zero-Point Energy Workflow Notebook
Introduction Notebook Link Updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Notebook as analyze_convex_hull.ipynb
participant API as Mat3ra API
participant Analysis as analysis.py
participant Plot as plot.py
User->>Notebook: Set FORMULAS, GROUP, PRECISION
Notebook->>API: Authenticate and select account
Notebook->>API: Search materials by formula
API-->>Notebook: Matched materials
Notebook->>API: Query total_energy properties
API-->>Notebook: Energy data per material
Notebook->>Notebook: Build entries_data (composition, atom count, energy)
Notebook->>Analysis: build_convex_hull(entries_data)
Analysis-->>Notebook: PhaseDiagram
Notebook->>Analysis: get_results_table(phase_diagram, entries_data)
Analysis-->>Notebook: DataFrame of stability results
Notebook->>Plot: plot_convex_hull(phase_diagram)
Plot-->>Notebook: Styled Plotly figure
Notebook-->>User: Display results table and interactive plot
sequenceDiagram
participant User
participant Notebook as zero_point_energy.ipynb
participant API as Mat3ra API
participant Cluster as Compute Cluster
User->>Notebook: Set material, workflow, and compute parameters
Notebook->>API: Authenticate and select account/project
Notebook->>Notebook: Load material and visualize
Notebook->>API: Save/create material
Notebook->>API: Load workflow from Standata
Notebook->>Notebook: Modify workflow (relaxation, model, k-grids)
Notebook->>API: Save/create workflow
Notebook->>API: List clusters and create compute config
Notebook->>API: Create job (material, workflow, project, compute)
Notebook->>Cluster: Submit job
Notebook->>API: Poll job status asynchronously
Cluster-->>API: Job completion
Notebook->>API: Fetch ZPE and total energy properties
Notebook-->>User: Visualize results
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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: 6
🧹 Nitpick comments (7)
src/py/mat3ra/notebooks_utils/ipython/entity/property/plot.py (1)
26-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilent skip when
hovertextmissing.Traces without
hovertextare skipped entirely (notext/textfontstyling applied), which means such points may render with default (possibly hard-to-read) styling against the dark theme rather than a fallback. Low risk given current PDPlotter always emits hovertext, but worth a defensive fallback if this changes.🤖 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/ipython/entity/property/plot.py` around lines 26 - 27, The hover-text guard in the plot styling logic is too strict and skips traces entirely when hovertext is absent, leaving them unstyled. Update the trace-processing path in the plot helper to use a defensive fallback for traces without hovertext so `text`/`textfont` styling still gets applied, keeping the behavior aligned with the existing `PDPlotter` flow and the trace styling code.src/py/mat3ra/notebooks_utils/core/entity/property/analysis.py (2)
50-55: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPositional pairing between
entries_dataandphase_diagram.all_entriesis unvalidated.
get_results_tableassumesentries_data[i]corresponds tophase_diagram.all_entries[i]in the same order used to build the hull. There's no length check or explicit ID-based matching; if a caller passes a differententries_datalist (or pymatgen reorders/dedupes entries internally), rows would silently show wrong Material ID/metadata paired with the wrong energetics.🤖 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/analysis.py` around lines 50 - 55, The pairing logic in get_results_table assumes entries_data and phase_diagram.all_entries stay in the same order, which can silently mismatch Material IDs and metadata. Update the function to validate that the two collections align before iterating, or match each phase_diagram entry to its corresponding entries_data item using a stable identifier from the entry/data objects instead of list position. Keep the existing results loop in get_results_table, but add a guard or lookup step around phase_diagram.all_entries, entries_data, and the per-row data assignment.
63-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMagic threshold
1e-6duplicated.Consider extracting
1e-6into a named constant (e.g.,STABILITY_TOLERANCE) for clarity and single source of truth.🤖 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/analysis.py` around lines 63 - 64, The stability check in the property analysis mapping uses the same 1e-6 threshold twice, so extract it into a single named constant such as STABILITY_TOLERANCE near the logic in the analysis function. Update both the “Stable” and “Decomposes to” conditions to reference that constant so the threshold has one source of truth and the intent is clearer.other/materials_designer/workflows/zero_point_energy.ipynb (1)
384-386: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
ADD_RELAXATIONternary.Line 384 already guards on
... and ADD_RELAXATION, soif ADD_RELAXATION else Noneon line 386 is always the truthy branch and can be simplified.♻️ Simplify
- new_context_relax = PointsGridDataProvider(dimensions=RELAXATION_KGRID, - isEdited=True).get_context_item_data() if ADD_RELAXATION else None + new_context_relax = PointsGridDataProvider(dimensions=RELAXATION_KGRID, + isEdited=True).get_context_item_data()🤖 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 384 - 386, Simplify the relaxation context setup by removing the redundant conditional in the zero_point_energy notebook snippet: the surrounding guard already checks RELAXATION_KGRID is not None and ADD_RELAXATION, so the expression assigned to new_context_relax should directly use PointsGridDataProvider(...).get_context_item_data() without an extra “if ADD_RELAXATION else None” branch. Update the logic near the ADD_RELAXATION check so the assignment is consistent with the existing guard and keeps the code easier to read.other/materials_designer/workflows/analyze_convex_hull.ipynb (3)
306-307: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBlanket warning suppression may hide legitimate hull-construction issues.
warnings.filterwarnings("ignore")suppresses all warnings for the rest of the kernel session, including potentially important pymatgen warnings (e.g. incomplete phase space, entries excluded from the hull). Consider scoping suppression to a specific category/module instead of a global blanket ignore.🤖 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/analyze_convex_hull.ipynb` around lines 306 - 307, The notebook currently uses a global warnings ignore that can hide important convex-hull and pymatgen issues. Update the warning handling in the analyze_convex_hull workflow so it is scoped to the specific warning category or module instead of calling warnings.filterwarnings("ignore") broadly. Keep the change near the existing warnings import/use so the rest of the kernel session is not silenced.
279-280: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winInconsistent defensive access vs. the preview cell.
The preview cell (lines 211-212) reads
material.get("basis", {}).get("coordinates", [])defensively, but this cell indexes directly withmaterial["basis"]["elements"]andmaterial["basis"]["coordinates"]. If any material inall_materialslacks abasiskey, this will raise an unhandledKeyErrorand abort the notebook mid-run instead of failing gracefully or reporting which material is malformed.🤖 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/analyze_convex_hull.ipynb` around lines 279 - 280, The access pattern in the material-processing cell is inconsistent with the defensive preview logic and can raise a KeyError when a material lacks basis data. Update the notebook cell that reads material["basis"]["elements"] and material["basis"]["coordinates"] to use the same safe get-based approach as the preview cell, ideally inside the loop over all_materials, and handle missing or malformed basis data gracefully while preserving the material context for any warning or skip.
236-277: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMaterials missing
exabyteIdare silently dropped with no feedback.Materials without an
exabyteId(line 270) are skipped silently, while materials with anexabyteIdbut no matching total-energy property do get a warning (⚠️ ... no total energy, line 276). If several materials from the search lackexabyteId(e.g. never sent to a job), the user gets no indication whyentries_dataends up small or empty, making downstream failures inbuild_convex_hullharder to diagnose.🩹 Proposed fix
for material in all_materials: exabyte_id = material.get("exabyteId") - if not exabyte_id or exabyte_id in seen_exabyte_ids: + if not exabyte_id: + print(f"⚠️ {material['formula']}: no exabyteId (never submitted to a job)") + continue + if exabyte_id in seen_exabyte_ids: continue seen_exabyte_ids.add(exabyte_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/analyze_convex_hull.ipynb` around lines 236 - 277, Materials without an exabyteId are being skipped silently in the entries collection loop, which makes it hard to understand why entries_data is smaller than expected. In the notebook cell that builds entries_data, update the loop around all_materials to explicitly detect missing exabyteId values and print a warning similar to the existing “no total energy” message. Use the same material iteration logic and the seen_exabyte_ids check, but add a clear notice before continuing so users can distinguish missing exabyteId cases from missing property cases.
🤖 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/Introduction.ipynb`:
- Around line 78-79: Remove the stale Formation Energy section placeholder from
the notebook’s introduction content so it no longer appears as a separate
navigation entry. Update the relevant markdown cells in Introduction.ipynb to
drop the `6.7. Formation Energy` heading and its `Compound formation energy`
subheading, since `Phase Stability` now covers that material. Keep the
surrounding section numbering consistent with the rest of the notebook and
ensure no duplicate TOC entry remains.
- Around line 75-76: The Introduction notebook TOC points to
generate_phase_diagram.ipynb, but that target is missing, so fix the broken link
by either adding the missing notebook or updating the entry to point to the
correct existing notebook. Check the table-of-contents entry in
Introduction.ipynb and the related workflow notebook names to ensure the phase
diagram section links to a valid target.
In `@other/materials_designer/workflows/zero_point_energy.ipynb`:
- Around line 334-335: The 4.3.2 note in zero_point_energy.ipynb is inconsistent
with the following model/method cell, which is currently active and runs by
default. Update the notebook so the instruction matches the implementation:
either comment out or otherwise gate the referenced model/method setup cell, or
revise the markdown text to state that the parameters are applied automatically.
Use the surrounding 4.3.2 markdown and the adjacent model/method cell to keep
the optional behavior clear.
- Around line 466-476: The cluster selection logic in the notebook can leave
cluster as None when CLUSTER_NAME does not match any entry, or crash with
IndexError when clusters is empty; add an explicit validation before
constructing Compute. Update the cluster-selection block that uses CLUSTER_NAME,
clusters, and Compute so it checks for an empty clusters list and for a failed
hostname match, then fail fast with a clear message instead of letting
compute.cluster.hostname blow up later.
In `@src/py/mat3ra/notebooks_utils/ipython/entity/property/plot.py`:
- Line 39: The Plotly text styling in plot.py uses trace.textfont.shadow, but
the current dependency floor still allows versions that do not support this
property. Update the compatibility by either raising the Plotly minimum version
requirement to 5.23+ or removing the shadow entry from the textfont dict in the
code that builds the trace.
- Around line 25-33: The label generation in the hover-text processing loop is
relying on the rendered string format from PDPlotter, which is brittle. Update
the logic in the figure/trace handling code that iterates over fig.data to
derive formula and energy/status from structured phase-diagram data or trace
metadata instead of splitting trace.hovertext. Use the relevant trace attributes
or source data passed into the plotting function to build labels directly, so
the display does not depend on pymatgen’s internal hover-text layout.
---
Nitpick comments:
In `@other/materials_designer/workflows/analyze_convex_hull.ipynb`:
- Around line 306-307: The notebook currently uses a global warnings ignore that
can hide important convex-hull and pymatgen issues. Update the warning handling
in the analyze_convex_hull workflow so it is scoped to the specific warning
category or module instead of calling warnings.filterwarnings("ignore") broadly.
Keep the change near the existing warnings import/use so the rest of the kernel
session is not silenced.
- Around line 279-280: The access pattern in the material-processing cell is
inconsistent with the defensive preview logic and can raise a KeyError when a
material lacks basis data. Update the notebook cell that reads
material["basis"]["elements"] and material["basis"]["coordinates"] to use the
same safe get-based approach as the preview cell, ideally inside the loop over
all_materials, and handle missing or malformed basis data gracefully while
preserving the material context for any warning or skip.
- Around line 236-277: Materials without an exabyteId are being skipped silently
in the entries collection loop, which makes it hard to understand why
entries_data is smaller than expected. In the notebook cell that builds
entries_data, update the loop around all_materials to explicitly detect missing
exabyteId values and print a warning similar to the existing “no total energy”
message. Use the same material iteration logic and the seen_exabyte_ids check,
but add a clear notice before continuing so users can distinguish missing
exabyteId cases from missing property cases.
In `@other/materials_designer/workflows/zero_point_energy.ipynb`:
- Around line 384-386: Simplify the relaxation context setup by removing the
redundant conditional in the zero_point_energy notebook snippet: the surrounding
guard already checks RELAXATION_KGRID is not None and ADD_RELAXATION, so the
expression assigned to new_context_relax should directly use
PointsGridDataProvider(...).get_context_item_data() without an extra “if
ADD_RELAXATION else None” branch. Update the logic near the ADD_RELAXATION check
so the assignment is consistent with the existing guard and keeps the code
easier to read.
In `@src/py/mat3ra/notebooks_utils/core/entity/property/analysis.py`:
- Around line 50-55: The pairing logic in get_results_table assumes entries_data
and phase_diagram.all_entries stay in the same order, which can silently
mismatch Material IDs and metadata. Update the function to validate that the two
collections align before iterating, or match each phase_diagram entry to its
corresponding entries_data item using a stable identifier from the entry/data
objects instead of list position. Keep the existing results loop in
get_results_table, but add a guard or lookup step around
phase_diagram.all_entries, entries_data, and the per-row data assignment.
- Around line 63-64: The stability check in the property analysis mapping uses
the same 1e-6 threshold twice, so extract it into a single named constant such
as STABILITY_TOLERANCE near the logic in the analysis function. Update both the
“Stable” and “Decomposes to” conditions to reference that constant so the
threshold has one source of truth and the intent is clearer.
In `@src/py/mat3ra/notebooks_utils/ipython/entity/property/plot.py`:
- Around line 26-27: The hover-text guard in the plot styling logic is too
strict and skips traces entirely when hovertext is absent, leaving them
unstyled. Update the trace-processing path in the plot helper to use a defensive
fallback for traces without hovertext so `text`/`textfont` styling still gets
applied, keeping the behavior aligned with the existing `PDPlotter` flow and the
trace styling code.
🪄 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: b381a712-8351-4402-8320-c0191ef5b56d
📒 Files selected for processing (6)
other/materials_designer/create_solid_solution.ipynbother/materials_designer/workflows/Introduction.ipynbother/materials_designer/workflows/analyze_convex_hull.ipynbother/materials_designer/workflows/zero_point_energy.ipynbsrc/py/mat3ra/notebooks_utils/core/entity/property/analysis.pysrc/py/mat3ra/notebooks_utils/ipython/entity/property/plot.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
🧹 Nitpick comments (7)
src/py/mat3ra/notebooks_utils/ipython/entity/property/plot.py (1)
26-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilent skip when
hovertextmissing.Traces without
hovertextare skipped entirely (notext/textfontstyling applied), which means such points may render with default (possibly hard-to-read) styling against the dark theme rather than a fallback. Low risk given current PDPlotter always emits hovertext, but worth a defensive fallback if this changes.🤖 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/ipython/entity/property/plot.py` around lines 26 - 27, The hover-text guard in the plot styling logic is too strict and skips traces entirely when hovertext is absent, leaving them unstyled. Update the trace-processing path in the plot helper to use a defensive fallback for traces without hovertext so `text`/`textfont` styling still gets applied, keeping the behavior aligned with the existing `PDPlotter` flow and the trace styling code.src/py/mat3ra/notebooks_utils/core/entity/property/analysis.py (2)
50-55: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPositional pairing between
entries_dataandphase_diagram.all_entriesis unvalidated.
get_results_tableassumesentries_data[i]corresponds tophase_diagram.all_entries[i]in the same order used to build the hull. There's no length check or explicit ID-based matching; if a caller passes a differententries_datalist (or pymatgen reorders/dedupes entries internally), rows would silently show wrong Material ID/metadata paired with the wrong energetics.🤖 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/analysis.py` around lines 50 - 55, The pairing logic in get_results_table assumes entries_data and phase_diagram.all_entries stay in the same order, which can silently mismatch Material IDs and metadata. Update the function to validate that the two collections align before iterating, or match each phase_diagram entry to its corresponding entries_data item using a stable identifier from the entry/data objects instead of list position. Keep the existing results loop in get_results_table, but add a guard or lookup step around phase_diagram.all_entries, entries_data, and the per-row data assignment.
63-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMagic threshold
1e-6duplicated.Consider extracting
1e-6into a named constant (e.g.,STABILITY_TOLERANCE) for clarity and single source of truth.🤖 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/analysis.py` around lines 63 - 64, The stability check in the property analysis mapping uses the same 1e-6 threshold twice, so extract it into a single named constant such as STABILITY_TOLERANCE near the logic in the analysis function. Update both the “Stable” and “Decomposes to” conditions to reference that constant so the threshold has one source of truth and the intent is clearer.other/materials_designer/workflows/zero_point_energy.ipynb (1)
384-386: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
ADD_RELAXATIONternary.Line 384 already guards on
... and ADD_RELAXATION, soif ADD_RELAXATION else Noneon line 386 is always the truthy branch and can be simplified.♻️ Simplify
- new_context_relax = PointsGridDataProvider(dimensions=RELAXATION_KGRID, - isEdited=True).get_context_item_data() if ADD_RELAXATION else None + new_context_relax = PointsGridDataProvider(dimensions=RELAXATION_KGRID, + isEdited=True).get_context_item_data()🤖 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 384 - 386, Simplify the relaxation context setup by removing the redundant conditional in the zero_point_energy notebook snippet: the surrounding guard already checks RELAXATION_KGRID is not None and ADD_RELAXATION, so the expression assigned to new_context_relax should directly use PointsGridDataProvider(...).get_context_item_data() without an extra “if ADD_RELAXATION else None” branch. Update the logic near the ADD_RELAXATION check so the assignment is consistent with the existing guard and keeps the code easier to read.other/materials_designer/workflows/analyze_convex_hull.ipynb (3)
306-307: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBlanket warning suppression may hide legitimate hull-construction issues.
warnings.filterwarnings("ignore")suppresses all warnings for the rest of the kernel session, including potentially important pymatgen warnings (e.g. incomplete phase space, entries excluded from the hull). Consider scoping suppression to a specific category/module instead of a global blanket ignore.🤖 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/analyze_convex_hull.ipynb` around lines 306 - 307, The notebook currently uses a global warnings ignore that can hide important convex-hull and pymatgen issues. Update the warning handling in the analyze_convex_hull workflow so it is scoped to the specific warning category or module instead of calling warnings.filterwarnings("ignore") broadly. Keep the change near the existing warnings import/use so the rest of the kernel session is not silenced.
279-280: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winInconsistent defensive access vs. the preview cell.
The preview cell (lines 211-212) reads
material.get("basis", {}).get("coordinates", [])defensively, but this cell indexes directly withmaterial["basis"]["elements"]andmaterial["basis"]["coordinates"]. If any material inall_materialslacks abasiskey, this will raise an unhandledKeyErrorand abort the notebook mid-run instead of failing gracefully or reporting which material is malformed.🤖 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/analyze_convex_hull.ipynb` around lines 279 - 280, The access pattern in the material-processing cell is inconsistent with the defensive preview logic and can raise a KeyError when a material lacks basis data. Update the notebook cell that reads material["basis"]["elements"] and material["basis"]["coordinates"] to use the same safe get-based approach as the preview cell, ideally inside the loop over all_materials, and handle missing or malformed basis data gracefully while preserving the material context for any warning or skip.
236-277: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMaterials missing
exabyteIdare silently dropped with no feedback.Materials without an
exabyteId(line 270) are skipped silently, while materials with anexabyteIdbut no matching total-energy property do get a warning (⚠️ ... no total energy, line 276). If several materials from the search lackexabyteId(e.g. never sent to a job), the user gets no indication whyentries_dataends up small or empty, making downstream failures inbuild_convex_hullharder to diagnose.🩹 Proposed fix
for material in all_materials: exabyte_id = material.get("exabyteId") - if not exabyte_id or exabyte_id in seen_exabyte_ids: + if not exabyte_id: + print(f"⚠️ {material['formula']}: no exabyteId (never submitted to a job)") + continue + if exabyte_id in seen_exabyte_ids: continue seen_exabyte_ids.add(exabyte_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/analyze_convex_hull.ipynb` around lines 236 - 277, Materials without an exabyteId are being skipped silently in the entries collection loop, which makes it hard to understand why entries_data is smaller than expected. In the notebook cell that builds entries_data, update the loop around all_materials to explicitly detect missing exabyteId values and print a warning similar to the existing “no total energy” message. Use the same material iteration logic and the seen_exabyte_ids check, but add a clear notice before continuing so users can distinguish missing exabyteId cases from missing property cases.
🤖 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/Introduction.ipynb`:
- Around line 78-79: Remove the stale Formation Energy section placeholder from
the notebook’s introduction content so it no longer appears as a separate
navigation entry. Update the relevant markdown cells in Introduction.ipynb to
drop the `6.7. Formation Energy` heading and its `Compound formation energy`
subheading, since `Phase Stability` now covers that material. Keep the
surrounding section numbering consistent with the rest of the notebook and
ensure no duplicate TOC entry remains.
- Around line 75-76: The Introduction notebook TOC points to
generate_phase_diagram.ipynb, but that target is missing, so fix the broken link
by either adding the missing notebook or updating the entry to point to the
correct existing notebook. Check the table-of-contents entry in
Introduction.ipynb and the related workflow notebook names to ensure the phase
diagram section links to a valid target.
In `@other/materials_designer/workflows/zero_point_energy.ipynb`:
- Around line 334-335: The 4.3.2 note in zero_point_energy.ipynb is inconsistent
with the following model/method cell, which is currently active and runs by
default. Update the notebook so the instruction matches the implementation:
either comment out or otherwise gate the referenced model/method setup cell, or
revise the markdown text to state that the parameters are applied automatically.
Use the surrounding 4.3.2 markdown and the adjacent model/method cell to keep
the optional behavior clear.
- Around line 466-476: The cluster selection logic in the notebook can leave
cluster as None when CLUSTER_NAME does not match any entry, or crash with
IndexError when clusters is empty; add an explicit validation before
constructing Compute. Update the cluster-selection block that uses CLUSTER_NAME,
clusters, and Compute so it checks for an empty clusters list and for a failed
hostname match, then fail fast with a clear message instead of letting
compute.cluster.hostname blow up later.
In `@src/py/mat3ra/notebooks_utils/ipython/entity/property/plot.py`:
- Line 39: The Plotly text styling in plot.py uses trace.textfont.shadow, but
the current dependency floor still allows versions that do not support this
property. Update the compatibility by either raising the Plotly minimum version
requirement to 5.23+ or removing the shadow entry from the textfont dict in the
code that builds the trace.
- Around line 25-33: The label generation in the hover-text processing loop is
relying on the rendered string format from PDPlotter, which is brittle. Update
the logic in the figure/trace handling code that iterates over fig.data to
derive formula and energy/status from structured phase-diagram data or trace
metadata instead of splitting trace.hovertext. Use the relevant trace attributes
or source data passed into the plotting function to build labels directly, so
the display does not depend on pymatgen’s internal hover-text layout.
---
Nitpick comments:
In `@other/materials_designer/workflows/analyze_convex_hull.ipynb`:
- Around line 306-307: The notebook currently uses a global warnings ignore that
can hide important convex-hull and pymatgen issues. Update the warning handling
in the analyze_convex_hull workflow so it is scoped to the specific warning
category or module instead of calling warnings.filterwarnings("ignore") broadly.
Keep the change near the existing warnings import/use so the rest of the kernel
session is not silenced.
- Around line 279-280: The access pattern in the material-processing cell is
inconsistent with the defensive preview logic and can raise a KeyError when a
material lacks basis data. Update the notebook cell that reads
material["basis"]["elements"] and material["basis"]["coordinates"] to use the
same safe get-based approach as the preview cell, ideally inside the loop over
all_materials, and handle missing or malformed basis data gracefully while
preserving the material context for any warning or skip.
- Around line 236-277: Materials without an exabyteId are being skipped silently
in the entries collection loop, which makes it hard to understand why
entries_data is smaller than expected. In the notebook cell that builds
entries_data, update the loop around all_materials to explicitly detect missing
exabyteId values and print a warning similar to the existing “no total energy”
message. Use the same material iteration logic and the seen_exabyte_ids check,
but add a clear notice before continuing so users can distinguish missing
exabyteId cases from missing property cases.
In `@other/materials_designer/workflows/zero_point_energy.ipynb`:
- Around line 384-386: Simplify the relaxation context setup by removing the
redundant conditional in the zero_point_energy notebook snippet: the surrounding
guard already checks RELAXATION_KGRID is not None and ADD_RELAXATION, so the
expression assigned to new_context_relax should directly use
PointsGridDataProvider(...).get_context_item_data() without an extra “if
ADD_RELAXATION else None” branch. Update the logic near the ADD_RELAXATION check
so the assignment is consistent with the existing guard and keeps the code
easier to read.
In `@src/py/mat3ra/notebooks_utils/core/entity/property/analysis.py`:
- Around line 50-55: The pairing logic in get_results_table assumes entries_data
and phase_diagram.all_entries stay in the same order, which can silently
mismatch Material IDs and metadata. Update the function to validate that the two
collections align before iterating, or match each phase_diagram entry to its
corresponding entries_data item using a stable identifier from the entry/data
objects instead of list position. Keep the existing results loop in
get_results_table, but add a guard or lookup step around
phase_diagram.all_entries, entries_data, and the per-row data assignment.
- Around line 63-64: The stability check in the property analysis mapping uses
the same 1e-6 threshold twice, so extract it into a single named constant such
as STABILITY_TOLERANCE near the logic in the analysis function. Update both the
“Stable” and “Decomposes to” conditions to reference that constant so the
threshold has one source of truth and the intent is clearer.
In `@src/py/mat3ra/notebooks_utils/ipython/entity/property/plot.py`:
- Around line 26-27: The hover-text guard in the plot styling logic is too
strict and skips traces entirely when hovertext is absent, leaving them
unstyled. Update the trace-processing path in the plot helper to use a defensive
fallback for traces without hovertext so `text`/`textfont` styling still gets
applied, keeping the behavior aligned with the existing `PDPlotter` flow and the
trace styling code.
🪄 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: b381a712-8351-4402-8320-c0191ef5b56d
📒 Files selected for processing (6)
other/materials_designer/create_solid_solution.ipynbother/materials_designer/workflows/Introduction.ipynbother/materials_designer/workflows/analyze_convex_hull.ipynbother/materials_designer/workflows/zero_point_energy.ipynbsrc/py/mat3ra/notebooks_utils/core/entity/property/analysis.pysrc/py/mat3ra/notebooks_utils/ipython/entity/property/plot.py
🛑 Comments failed to post (6)
other/materials_designer/workflows/Introduction.ipynb (2)
75-76: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash fd -a 'generate_phase_diagram.ipynb' other/materials_designer/workflowsRepository: mat3ra/api-examples
Length of output: 157
🏁 Script executed:
#!/bin/bash set -euo pipefail # Check whether the target notebook exists anywhere in the repo. fd -a 'generate_phase_diagram.ipynb' . # Inspect nearby workflow notebooks and references to the target. rg -n "generate_phase_diagram\.ipynb|Phase Diagram" other/materials_designer/workflows -S # Show the relevant section of the introduction notebook. python3 - <<'PY' from pathlib import Path p = Path('other/materials_designer/workflows/Introduction.ipynb') data = p.read_text() for i, line in enumerate(data.splitlines(), 1): if 'generate_phase_diagram.ipynb' in line or 'Phase Diagram' in line or 'Formation Energies and Convex Hull' in line: print(f"{i}: {line}") PYRepository: mat3ra/api-examples
Length of output: 444
🏁 Script executed:
#!/bin/bash set -euo pipefail # Find all notebooks in the workflows area to see whether the TOC points to a matching file name. fd -a -e ipynb other/materials_designer/workflows # Show any references to the exact notebook name across the repo. rg -n "generate_phase_diagram\.ipynb" .Repository: mat3ra/api-examples
Length of output: 599
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect repository paths that might use a different casing or spelling for the phase diagram notebook. fd -a -i 'phase.*diagram.*ipynb' other/materials_designerRepository: mat3ra/api-examples
Length of output: 157
Add the missing phase diagram notebook target.
other/materials_designer/workflows/Introduction.ipynb:76links togenerate_phase_diagram.ipynb, but that file is absent fromother/materials_designer/workflows, so the TOC entry will be broken unless the notebook is added or the link is updated.🤖 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/Introduction.ipynb` around lines 75 - 76, The Introduction notebook TOC points to generate_phase_diagram.ipynb, but that target is missing, so fix the broken link by either adding the missing notebook or updating the entry to point to the correct existing notebook. Check the table-of-contents entry in Introduction.ipynb and the related workflow notebook names to ensure the phase diagram section links to a valid target.
78-79: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the stale Formation Energy heading.
Phase Stabilityalready covers the formation-energy / convex-hull material. Keeping a separate6.7. Formation Energyplaceholder leaves duplicate navigation and conflicts with the rename in this update.🤖 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/Introduction.ipynb` around lines 78 - 79, Remove the stale Formation Energy section placeholder from the notebook’s introduction content so it no longer appears as a separate navigation entry. Update the relevant markdown cells in Introduction.ipynb to drop the `6.7. Formation Energy` heading and its `Compound formation energy` subheading, since `Phase Stability` now covers that material. Keep the surrounding section numbering consistent with the rest of the notebook and ensure no duplicate TOC entry remains.other/materials_designer/workflows/zero_point_energy.ipynb (2)
334-335: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Markdown says "Uncomment the code below," but the code cell is active.
The instruction in 4.3.2 implies the following model/method cell is commented out, but it runs unconditionally. Either comment out the code (to make it truly optional) or update the markdown to reflect that it executes by default.
🤖 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 334 - 335, The 4.3.2 note in zero_point_energy.ipynb is inconsistent with the following model/method cell, which is currently active and runs by default. Update the notebook so the instruction matches the implementation: either comment out or otherwise gate the referenced model/method setup cell, or revise the markdown text to state that the parameters are applied automatically. Use the surrounding 4.3.2 markdown and the adjacent model/method cell to keep the optional behavior clear.
466-476: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against empty/unmatched cluster selection.
If
CLUSTER_NAMEis set but matches nothing,clusterbecomesNone; ifclustersis empty,clusters[0]raisesIndexError. Either path leads to a confusing failure atcompute.cluster.hostname. Add an explicit check.🛡️ Proposed guard
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}' found in {[c['hostname'] for c in clusters]}") else: + if not clusters: + raise ValueError("No clusters available for the selected account.") 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."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 matching '{CLUSTER_NAME}' found in {[c['hostname'] for c in clusters]}\")\n", "else:\n", " if not clusters:\n", " raise ValueError(\"No clusters available for the selected account.\")\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 466 - 476, The cluster selection logic in the notebook can leave cluster as None when CLUSTER_NAME does not match any entry, or crash with IndexError when clusters is empty; add an explicit validation before constructing Compute. Update the cluster-selection block that uses CLUSTER_NAME, clusters, and Compute so it checks for an empty clusters list and for a failed hostname match, then fail fast with a clear message instead of letting compute.cluster.hostname blow up later.src/py/mat3ra/notebooks_utils/ipython/entity/property/plot.py (2)
25-33: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does pymatgen PDPlotter hover text format for get_plot (plotly backend) remain stable across recent versions (2024+)?💡 Result:
The hover text format in the pymatgen PDPlotter plotly backend is not strictly guaranteed to remain stable across versions, as it is subject to ongoing updates, feature enhancements, and bug fixes [1][2]. While there is no explicit policy guaranteeing long-term stability of the hover text string format, the underlying code for generating these plots has received multiple functional updates between 2024 and 2026 [1][3][4]. Key points regarding stability and changes since 2024: 1. Functional Updates and Bug Fixes: The plotting logic in PDPlotter has been actively maintained. For instance, recent changes have addressed how entries are selected and displayed (e.g., ensuring only lowest-energy unstable compositions are shown), which directly affects the content of the hover information [4][2]. 2. Dependency-Driven Changes: The plotly backend itself is sensitive to changes in the underlying plotly library. For example, recent versions of pymatgen (incorporating PR
#4292) required updates to maintain compatibility with plotly v6, which involved refactoring layout properties [3][4]. Such changes can influence how visual elements, including hover labels, are rendered or configured [3]. 3. Nature of Implementation: The hover text is generated programmatically within the get_plot method [5]. Because this code is part of the library's active codebase, it is prone to modification when new features (like unary plots or 2D ternary support) are added or when existing rendering logic is refactored for better performance or correctness [1]. Users relying on parsing or automating the extraction of data from these hover tooltips should be aware that the exact string formatting or structure may change in future releases [1][2]. For critical workflows, it is recommended to pin the version of pymatgen used, or programmatically extract the underlying data directly from the PhaseDiagram object rather than relying on the plot's hover output [6].Citations:
- 1: materialsproject/pymatgen#3032
- 2: materialsproject/pymatgen#4377
- 3: materialsproject/pymatgen#4292
- 4: https://github.com/materialsproject/pymatgen/blob/master/docs/CHANGES.md
- 5: https://github.com/materialsproject/pymatgen-core/blob/v2026.5.18/src/pymatgen/analysis/phase_diagram.py
- 6: https://docs.materialsproject.org/methodology/materials-methodology/thermodynamic-stability/phase-diagrams-pds
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate the reviewed file and nearby dependency pinning/config references. git ls-files | rg '^src/py/mat3ra/notebooks_utils/ipython/entity/property/plot\.py$|(^|/)(pyproject\.toml|requirements.*|poetry\.lock|uv\.lock|Pipfile.lock|environment\.yml)$' || true echo echo "=== reviewed file ===" cat -n src/py/mat3ra/notebooks_utils/ipython/entity/property/plot.py echo echo "=== pymatgen references ===" rg -n "PDPlotter|pymatgen" src/py -S || trueRepository: mat3ra/api-examples
Length of output: 6560
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== pyproject pymatgen pin ===" rg -n "pymatgen|plotly" pyproject.toml -n -S || true echo echo "=== pyproject snippet ===" sed -n '1,220p' pyproject.tomlRepository: mat3ra/api-examples
Length of output: 3772
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path import re text = Path('src/py/mat3ra/notebooks_utils/ipython/entity/property/plot.py').read_text() m = re.search(r'for hover_text in trace\.hovertext:(.*?)trace\.text = tuple\(labels\)', text, re.S) print(m.group(1).strip() if m else "no match") PYRepository: mat3ra/api-examples
Length of output: 380
Avoid parsing
PDPlotterhover strings for labels.
This relies on pymatgen’s internal hover-text layout, so a future update can silently change the displayed formula/energy text. Use structured phase-diagram data or trace metadata instead of splitting the rendered string.🤖 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/ipython/entity/property/plot.py` around lines 25 - 33, The label generation in the hover-text processing loop is relying on the rendered string format from PDPlotter, which is brittle. Update the logic in the figure/trace handling code that iterates over fig.data to derive formula and energy/status from structured phase-diagram data or trace metadata instead of splitting trace.hovertext. Use the relevant trace attributes or source data passed into the plotting function to build labels directly, so the display does not depend on pymatgen’s internal hover-text layout.
39-39: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Plotly Python textfont shadow property support scatter trace💡 Result:
Yes, Plotly for Python supports the shadow property within the textfont attribute for scatter traces [1][2]. This feature, introduced in Plotly 5.23, allows you to apply a shadow effect to text [1]. The shadow property functions similarly to the CSS text-shadow property [1][2]. You can define it within the textfont dictionary as follows [2]: import plotly.graph_objects as go fig = go.Figure( data=go.Scatter( x=[3][1], y=[3], mode="text", text=["Label A", "Label B", "Label C"], textfont=dict( shadow="1px 1px 2px black"))) The shadow property accepts string values defining the shadow's offset, blur radius, and color [3][2]. Additionally, a value of "auto" can be used to apply a minimal shadow with a contrasting font color automatically [3][4]. Note that while standard scatter traces support this property, support can vary for other trace types; for example, it has been reported as missing for some specialized map traces like Scattermap [5].
Citations:
- 1: https://plotly.com/python/text-and-annotations/
- 2: https://github.com/plotly/plotly.py/blob/main/doc/python/text-and-annotations.md
- 3: https://plotly.com/python/reference/scatter/
- 4: https://plotly.com/python/reference/
- 5: plotly/plotly.js#7419
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== file context ==\n' cat -n src/py/mat3ra/notebooks_utils/ipython/entity/property/plot.py | sed -n '1,120p' printf '\n== plotly version references ==\n' rg -n --hidden --glob '!**/.git/**' 'plotly|Plotly' pyproject.toml poetry.lock requirements*.txt setup.* src/py -g '!**/__pycache__/**' || true printf '\n== around the relevant lines ==\n' sed -n '25,55p' src/py/mat3ra/notebooks_utils/ipython/entity/property/plot.pyRepository: mat3ra/api-examples
Length of output: 6019
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== plotly scatter textfont docs references in repo, if any ==\n' rg -n 'textfont.*shadow|shadow.*textfont|TEXT_SHADOW' src/py || true printf '\n== dependency files mentioning plotly ==\n' fd -a 'pyproject.toml|requirements*.txt|poetry.lock|Pipfile.lock|setup.cfg|setup.py' . 2>/dev/null | sed -n '1,20p'Repository: mat3ra/api-examples
Length of output: 804
Plotly
textfont.shadowneeds a 5.23+ floor insrc/py/mat3ra/notebooks_utils/ipython/entity/property/plot.pyplotly>=5.18still allows versions that don’t supporttextfont.shadow, so this can fail on the declared minimum. Either raise the dependency floor to 5.23+ or drop theshadowkey.🤖 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/ipython/entity/property/plot.py` at line 39, The Plotly text styling in plot.py uses trace.textfont.shadow, but the current dependency floor still allows versions that do not support this property. Update the compatibility by either raising the Plotly minimum version requirement to 5.23+ or removing the shadow entry from the textfont dict in the code that builds the trace.
1 participant
Summary by CodeRabbit