SOF-7777 Update: add python code to get VBO polar#265
SOF-7777 Update: add python code to get VBO polar#265
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces and tightens package pins in a requirements file and adds a new workflow type-stub Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User
participant Script as vbo_polar.pyi
participant Reader as StructureReader
participant Checkpoint as CheckpointLoader
participant Fitter as FittingRoutine
participant Plotter as Plotter
rect rgba(200,200,255,0.5)
User->>Script: invoke with 3 structure files + checkpoint
end
rect rgba(200,255,200,0.5)
Script->>Reader: load three pw_scf.out-like structures
Reader-->>Script: atomic positions & cell (cartesian)
Script->>Script: detect slab z-boundaries
end
rect rgba(255,200,200,0.5)
Script->>Checkpoint: load macroscopic potential profiles (X,Y)
Checkpoint-->>Script: X/Y for interface, bulk_left, bulk_right
end
rect rgba(255,230,200,0.5)
Script->>Fitter: call get_region_indices & fit_and_average for regions
Fitter-->>Script: region indices, averages, slopes, intercepts
Script->>Script: compute Va/Vb and VBO
end
rect rgba(200,255,255,0.5)
Script->>Plotter: draw profile, fits, regions
Plotter-->>Script: saved PNG
Script-->>User: print VBO and file path
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🤖 Fix all issues with AI agents
In `@other/materials_designer/workflows/requirements-with-made.txt`:
- Around line 1-5: Update the mat3ra-made version constraint in
requirements-with-made.txt so it references a real PyPI release instead of the
non-existent "2024.11.12.0"; replace the current "mat3ra-made >= 2024.11.12.0"
spec with a valid release such as "mat3ra-made >= 2024.11.26.post0" (or another
listed existing release like 2024.11.23.post0) to ensure the package can be
installed.
In `@other/materials_designer/workflows/vbo_polar.pyi`:
- Around line 61-67: The helper get_region_indices currently returns (0,
len(x_data)) when no points fall in [x_min, x_max], which silently causes
full-profile fits; change get_region_indices to fail fast by raising a
ValueError with a clear message including x_min and x_max (e.g., "empty region
for x_min=..., x_max=...") instead of returning a full-range sentinel; apply the
same change to the other identical implementation around lines 94-100 so callers
cannot accidentally fit the entire profile when a region is empty.
| munch == 2.5.0 | ||
| numpy >= 1.19.5 | ||
| scipy >= 1.5.4 | ||
| matplotlib >= 3.0.0 | ||
| mat3ra-made >= 2024.11.12.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import json, urllib.request
pkgs = {
"munch": "2.5.0",
"numpy": "1.19.5",
"scipy": "1.5.4",
"matplotlib": "3.0.0",
"mat3ra-made": "2024.11.12.0",
}
for pkg, min_ver in pkgs.items():
url = f"https://pypi.org/pypi/{pkg}/json"
try:
data = json.load(urllib.request.urlopen(url))
except Exception as e:
print(f"{pkg}: unable to query PyPI ({e})")
continue
latest = data["info"]["version"]
exists = min_ver in data.get("releases", {})
print(f"{pkg}: latest={latest}, min_spec={min_ver}, exists_on_pypi={exists}")
PYRepository: Exabyte-io/api-examples
Length of output: 153
🌐 Web query:
mat3ra-made PyPI package 2024.11.12.0
💡 Result:
I couldn't find a PyPI release named exactly "mat3ra-made 2024.11.12.0". The mat3ra-made PyPI project page and piwheels release list show multiple November 2024 releases (e.g. 2024.11.1.post0, 2024.11.6.post0, 2024.11.7.post0, 2024.11.16.post0, 2024.11.19.post0, 2024.11.23.post0, 2024.11.26.post0) but not 2024.11.12.0. [1][2]
Sources:
- mat3ra-made on PyPI. [1]
- mat3ra-made release list on piwheels. [2]
🌐 Web query:
numpy 1.19.5 scipy 1.5.4 matplotlib 3.0.0 compatibility
💡 Result:
Short answer: compatible.
- SciPy 1.5.x requires NumPy >=1.14.5 and <1.20.0, so NumPy 1.19.5 is within the supported range (SciPy 1.5.4 will work with NumPy 1.19.5). [1]
- Matplotlib 3.0.0 has a very low NumPy minimum (>=1.10.0), so NumPy 1.19.5 is fine for Matplotlib 3.0.0. [2]
Note: SciPy 1.5.x was targeted at Python 3.6–3.9-era runtimes, so ensure your Python version is also compatible with those package versions. [1]
Sources:
[1] SciPy toolchain / version support (SciPy 1.5: NumPy >=1.14.5, <1.20.0).
[2] Matplotlib minimum dependency table (Matplotlib 3.0: NumPy >=1.10.0).
🌐 Web query:
munch 2.5.0 PyPI
💡 Result:
munch 2.5.0 is a published PyPI release (uploaded Oct 30, 2019). You can install it with:
pip install munch==2.5.0. [1][2]
Sources:
[1] PyPI project page (release history).
[2] PyPI release listing for munch (shows 2.5.0, Oct 30, 2019).
Fix mat3ra-made version constraint—2024.11.12.0 does not exist on PyPI.
The specified version mat3ra-made >= 2024.11.12.0 cannot be installed; this version is not published on PyPI. Available November 2024 releases include 2024.11.1.post0, 2024.11.6.post0, 2024.11.7.post0, 2024.11.16.post0, 2024.11.19.post0, 2024.11.23.post0, and 2024.11.26.post0. Update the constraint to use an existing version.
The other dependencies (munch 2.5.0, numpy 1.19.5, scipy 1.5.4, matplotlib 3.0.0) all exist on PyPI and are mutually compatible.
🤖 Prompt for AI Agents
In `@other/materials_designer/workflows/requirements-with-made.txt` around lines 1
- 5, Update the mat3ra-made version constraint in requirements-with-made.txt so
it references a real PyPI release instead of the non-existent "2024.11.12.0";
replace the current "mat3ra-made >= 2024.11.12.0" spec with a valid release such
as "mat3ra-made >= 2024.11.26.post0" (or another listed existing release like
2024.11.23.post0) to ensure the package can be installed.
| def get_region_indices(x_data, x_min, x_max): | ||
| """Get array indices corresponding to coordinate range.""" | ||
| mask = (x_data >= x_min) & (x_data <= x_max) | ||
| indices = np.where(mask)[0] | ||
| if len(indices) == 0: | ||
| return 0, len(x_data) | ||
| return indices[0], indices[-1] + 1 |
There was a problem hiding this comment.
Don’t fit the entire profile when a region is empty.
Returning (0, len(x_data)) on empty regions silently fits the full profile and produces incorrect Va/Vb when the region bounds are wrong. Fail fast (or explicitly return empty) instead.
🐛 Proposed fix (fail fast on empty regions)
def get_region_indices(x_data, x_min, x_max):
"""Get array indices corresponding to coordinate range."""
mask = (x_data >= x_min) & (x_data <= x_max)
indices = np.where(mask)[0]
if len(indices) == 0:
- return 0, len(x_data)
+ return 0, 0
return indices[0], indices[-1] + 1
# Get indices for each slab region
slab1_start, slab1_end = get_region_indices(X, z_min_1, z_max_1)
slab2_start, slab2_end = get_region_indices(X, z_min_2, z_max_2)
+
+if slab1_end <= slab1_start:
+ raise ValueError(f"No points found in slab 1 region ({z_min_1}, {z_max_1}).")
+if slab2_end <= slab2_start:
+ raise ValueError(f"No points found in slab 2 region ({z_min_2}, {z_max_2}).")Also applies to: 94-100
🤖 Prompt for AI Agents
In `@other/materials_designer/workflows/vbo_polar.pyi` around lines 61 - 67, The
helper get_region_indices currently returns (0, len(x_data)) when no points fall
in [x_min, x_max], which silently causes full-profile fits; change
get_region_indices to fail fast by raising a ValueError with a clear message
including x_min and x_max (e.g., "empty region for x_min=..., x_max=...")
instead of returning a full-range sentinel; apply the same change to the other
identical implementation around lines 94-100 so callers cannot accidentally fit
the entire profile when a region is empty.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@other/materials_designer/workflows/vbo_polar.pyi`:
- Around line 95-107: In fit_and_average, do not silently return 0.0 for an
empty region; instead detect when x_region and y_region are empty and raise a
ValueError describing the empty fit region, and only use the "avg with zero
slope" fallback when the region contains exactly one point (compute avg =
float(np.mean(y_region)) and return avg, 0.0, avg); keep the regular linear fit
logic for len(x_region) >= 2. Ensure the error message mentions fit_and_average
and the start/end indices for debugging.
- Around line 1-79: This file is a runtime script (it performs imports, file I/O
and computes X/Y from profile_interface) but is saved as a type stub (.pyi);
rename the file from .pyi to .py so Python will execute it at runtime, and
ensure any consumers import the module by its new name; verify symbols like
Material.create, from_ase, ase.io.read and the variables pw_scf_output_1/2,
profile_interface, profile_left/right, and X/Y continue to work after renaming
and update any references or imports elsewhere to point to the new module name.
♻️ Duplicate comments (1)
other/materials_designer/workflows/vbo_polar.pyi (1)
87-93: Fail fast on empty regions instead of fitting the full profile.Returning
(0, len(x_data))on empty regions silently fits the entire profile and yields incorrect Va/Vb. Prefer raising a clear error when no points are found.🐛 Proposed fix (fail fast)
def get_region_indices(x_data, x_min, x_max): """Get array indices corresponding to coordinate range.""" mask = (x_data >= x_min) & (x_data <= x_max) indices = np.where(mask)[0] if len(indices) == 0: - return 0, len(x_data) + raise ValueError(f"empty region for x_min={x_min}, x_max={x_max}") return indices[0], indices[-1] + 1
| def fit_and_average(x_data, y_data, start_idx, end_idx): | ||
| """ | ||
| Fit linear regression to region and return average value, slope, and intercept. | ||
|
|
||
| The average of the fitted line equals the mean of y-values, | ||
| but fitting helps smooth out oscillations and validates linearity. | ||
| """ | ||
| x_region = x_data[start_idx:end_idx] | ||
| y_region = y_data[start_idx:end_idx] | ||
|
|
||
| if len(x_region) < 2: | ||
| avg = float(np.mean(y_region)) if len(y_region) > 0 else 0.0 | ||
| return avg, 0.0, avg |
There was a problem hiding this comment.
Avoid silent 0.0 when the fit region is empty.
Returning 0.0 for an empty region can silently skew the VBO. Consider raising a ValueError on empty regions and only using the “avg with zero slope” fallback for exactly one point.
🐛 Proposed fix (guard empty region)
def fit_and_average(x_data, y_data, start_idx, end_idx):
@@
x_region = x_data[start_idx:end_idx]
y_region = y_data[start_idx:end_idx]
- if len(x_region) < 2:
- avg = float(np.mean(y_region)) if len(y_region) > 0 else 0.0
- return avg, 0.0, avg
+ if len(x_region) == 0:
+ raise ValueError(f"empty fit region for indices [{start_idx}, {end_idx})")
+ if len(x_region) == 1:
+ avg = float(y_region[0])
+ return avg, 0.0, avg🤖 Prompt for AI Agents
In `@other/materials_designer/workflows/vbo_polar.pyi` around lines 95 - 107, In
fit_and_average, do not silently return 0.0 for an empty region; instead detect
when x_region and y_region are empty and raise a ValueError describing the empty
fit region, and only use the "avg with zero slope" fallback when the region
contains exactly one point (compute avg = float(np.mean(y_region)) and return
avg, 0.0, avg); keep the regular linear fit logic for len(x_region) >= 2. Ensure
the error message mentions fit_and_average and the start/end indices for
debugging.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
other/materials_designer/workflows/vbo_polar.pyi (2)
45-47: Unnecessary f-string prefixes for static paths.These paths contain no interpolated expressions, so the
fprefix is redundant.♻️ Suggested simplification
-pw_scf_output = f"./pw_scf.out" -pw_scf_output_1 = f"./pw_scf.out-1" -pw_scf_output_2 = f"./pw_scf.out-2" +pw_scf_output = "./pw_scf.out" +pw_scf_output_1 = "./pw_scf.out-1" +pw_scf_output_2 = "./pw_scf.out-2"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@other/materials_designer/workflows/vbo_polar.pyi` around lines 45 - 47, The three variables pw_scf_output, pw_scf_output_1, and pw_scf_output_2 use f-string syntax unnecessarily for static file paths; remove the leading f from each assignment so they are plain string literals (e.g., "./pw_scf.out", "./pw_scf.out-1", "./pw_scf.out-2") to simplify the code and avoid misleading interpolation intent.
191-191: Unnecessary f-string prefix.Same as the earlier file paths—no interpolation here.
♻️ Suggested simplification
-filename = f"polar_vbo_fit_interface.png" +filename = "polar_vbo_fit_interface.png"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@other/materials_designer/workflows/vbo_polar.pyi` at line 191, The assignment to the variable filename uses an unnecessary f-string: update the assignment of filename (symbol "filename") to a plain string literal without the f-prefix so it reads a normal string (e.g., "polar_vbo_fit_interface.png") rather than f"polar_vbo_fit_interface.png"; this removes redundant interpolation and matches other file path declarations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@other/materials_designer/workflows/vbo_polar.pyi`:
- Around line 67-72: The code computes z_min_2 using z_elements[n_left] without
ensuring n_left < len(z_elements), which can raise IndexError when
material_1.basis has the same number of atoms as the combined interface; update
the logic around n_left, z_elements, z_max_1, z_min_2 (and related z_min_1,
z_max_2) to first check that n_left < len(z_elements) and handle the boundary
case (either by raising a clear ValueError with context including material_1 and
z_elements lengths, or by adjusting the slicing logic to safely pick the first
right-slab element only when present), so the function fails gracefully or uses
a safe fallback instead of throwing an unhandled IndexError.
---
Nitpick comments:
In `@other/materials_designer/workflows/vbo_polar.pyi`:
- Around line 45-47: The three variables pw_scf_output, pw_scf_output_1, and
pw_scf_output_2 use f-string syntax unnecessarily for static file paths; remove
the leading f from each assignment so they are plain string literals (e.g.,
"./pw_scf.out", "./pw_scf.out-1", "./pw_scf.out-2") to simplify the code and
avoid misleading interpolation intent.
- Line 191: The assignment to the variable filename uses an unnecessary
f-string: update the assignment of filename (symbol "filename") to a plain
string literal without the f-prefix so it reads a normal string (e.g.,
"polar_vbo_fit_interface.png") rather than f"polar_vbo_fit_interface.png"; this
removes redundant interpolation and matches other file path declarations.
| n_left = len(material_1.basis.elements.values) | ||
|
|
||
| z_max_1 = z_elements[n_left - 1][0] # Last atom of left slab | ||
| z_min_2 = z_elements[n_left][0] # First atom of right slab | ||
| z_min_1 = z_elements[0][0] | ||
| z_max_2 = z_elements[-1][0] |
There was a problem hiding this comment.
Missing bounds check could cause IndexError.
If material_1 has the same number of atoms as the interface material (i.e., n_left == len(z_elements)), accessing z_elements[n_left] on line 70 will raise an IndexError. Consider adding a guard.
🛡️ Proposed fix
n_left = len(material_1.basis.elements.values)
+
+if n_left >= len(z_elements):
+ raise ValueError(
+ f"Left slab atom count ({n_left}) >= interface atom count ({len(z_elements)}). "
+ "Cannot determine slab boundaries."
+ )
z_max_1 = z_elements[n_left - 1][0] # Last atom of left slab
z_min_2 = z_elements[n_left][0] # First atom of right slab🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@other/materials_designer/workflows/vbo_polar.pyi` around lines 67 - 72, The
code computes z_min_2 using z_elements[n_left] without ensuring n_left <
len(z_elements), which can raise IndexError when material_1.basis has the same
number of atoms as the combined interface; update the logic around n_left,
z_elements, z_max_1, z_min_2 (and related z_min_1, z_max_2) to first check that
n_left < len(z_elements) and handle the boundary case (either by raising a clear
ValueError with context including material_1 and z_elements lengths, or by
adjusting the slicing logic to safely pick the first right-slab element only
when present), so the function fails gracefully or uses a safe fallback instead
of throwing an unhandled IndexError.
2 participants
Summary by CodeRabbit
New Features
Chores