find_equilibrium_config with reverse-scan - #42
Conversation
📝 WalkthroughWalkthroughThe FCC equilibrium workflow now uses fixed reference configurations, crash-safe reverse coarse scans, isolated Nelder–Mead refinement, structured failure results, updated end-to-end and timeout tests, and a macOS Homebrew trust step. ChangesFCC equilibration workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant find_equilibrium_config_FCC
participant coarse_scan_reverse_safe
participant scipy_nelder_mead_safe
Caller->>find_equilibrium_config_FCC: request FCC equilibration
find_equilibrium_config_FCC->>coarse_scan_reverse_safe: scan reference configuration
coarse_scan_reverse_safe-->>find_equilibrium_config_FCC: candidate minima or failure
find_equilibrium_config_FCC->>scipy_nelder_mead_safe: refine candidates
scipy_nelder_mead_safe-->>find_equilibrium_config_FCC: equilibrium result or failure
find_equilibrium_config_FCC-->>Caller: minimal or full structured result
Poem
🚥 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 |
|
Pending issue - Some Buckingham models do not have a monospecies equilibrium. In such cases a mixed-species configuration must sweep the entire range of At least one BOP returns a NaN and crashes the VC:
UPDATE: This was already implemented. |
|
Implement a FLAG to choose whether or not we want to compute the monospecies configuration.
NOTE: This is pending feedback from Ellad. We might choose to go the alternate route and instead extend the 3 VCs to also test monospecies configurations in which case this FLAG will not be implemented. UPDATE: We've decided to go the opposite way. We shall extend the 3 VCs to check for monospecies configurations. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #42 +/- ##
==========================================
- Coverage 72.67% 68.87% -3.81%
==========================================
Files 13 13
Lines 2734 2978 +244
==========================================
+ Hits 1987 2051 +64
- Misses 747 927 +180 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
kim_tools/ase/core.py (1)
1401-1425: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
find_peaksmisses boundary minima; consider a global-argmin fallback.
find_peaksnever reports the first or last sample, so a scan truncated by a crash/timeout right at the basin — or a minimum ata_stop/a_start— yields zero starting points and_equilibrate_one_configfails withno_coarse_minima_found. A cheap fallback is to add the in-bounds global minimum when no interior peak is found. Note also that failed alats are dropped from the arrays, so points flanking a failure are treated as adjacent.🤖 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 `@kim_tools/ase/core.py` around lines 1401 - 1425, The _local_minima_indices function should fall back to an in-bounds global minimum when find_peaks returns no interior minima, including valid boundary minima at the first or last sample. Ignore non-finite energies when selecting the fallback, and preserve the existing empty/all-nonfinite behavior and integer index output.
🤖 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 @.github/workflows/testing.yml:
- Around line 52-53: Remove the brew trust invocation from the macOS setup in
the workflow and install AFLOW directly using the fully qualified formula
reference. Preserve the existing installation behavior while ensuring the macOS
15 runner does not depend on the unavailable brew trust command.
In `@kim_tools/ase/core.py`:
- Around line 946-951: Resolve the Python-version mismatch in core.py
annotations: either raise pyproject.toml’s requires-python minimum to Python 3.9
or newer, or make all bare list[T]/tuple[T, T] annotations runtime-compatible
with Python 3.8 using postponed annotations or typing wrappers. Ensure the
chosen approach covers annotations throughout the module, including
_species_label and _round_alat.
- Around line 1111-1120: Remove the explicit calc.__del__() invocation and its
surrounding cleanup block, leaving the existing del calc cleanup and atoms.calc
= None assignment intact so finalization occurs only through normal object
lifecycle handling.
- Around line 2142-2197: Forward the requested ncells_per_side parameter to both
_equilibrate_one_config calls instead of hardcoding 1. Set
full_result["ncells_per_side"] from the actual successful final_result
good_ncells, falling back to the reference configuration when needed, so the
reported value reflects the generated cell rather than the requested argument.
- Around line 1084-1085: Move the KIM calculator construction and assignment in
the relevant helper into the existing try block, including both KIM(model) and
atoms.calc = calc. Preserve the documented None-on-failure behavior so
model-load errors are caught rather than propagated to _coarse_scan_worker or
the Nelder–Mead objective.
- Around line 1702-1742: Update the process coordination around proc.start() in
scipy_nelder_mead_safe to drain result_queue before calling proc.join(timeout),
preventing large successful payloads from blocking child shutdown. Store the
retrieved result, then join and preserve the existing timeout and exitcode
handling; remove the later result_queue.get(timeout=1.0) and use the
already-retrieved result for subsequent validation.
In `@tests/test_ase.py`:
- Around line 31-36: Update find_equilibrium_config_FCC so both mono-species and
mixed-species calls to _equilibrate_one_config pass the caller-provided
ncells_per_side instead of a hardcoded 1. Add a regression test using a
non-default cell count that verifies the generated configuration and
result["ncells_per_side"] match the requested value.
---
Nitpick comments:
In `@kim_tools/ase/core.py`:
- Around line 1401-1425: The _local_minima_indices function should fall back to
an in-bounds global minimum when find_peaks returns no interior minima,
including valid boundary minima at the first or last sample. Ignore non-finite
energies when selecting the fallback, and preserve the existing
empty/all-nonfinite behavior and integer index output.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ff17015f-23c8-482b-996b-7e245611344e
📒 Files selected for processing (3)
.github/workflows/testing.ymlkim_tools/ase/core.pytests/test_ase.py
✅ Action performedReview finished.
|
ilia-nikiforov-umn
left a comment
ilia-nikiforov-umn
left a comment
There was a problem hiding this comment.
Very nicely written and documented overall, I think I was able to understand all the logic just by going through the code. Just a couple of comments below that need to be addressed.
only calc.clean needs to be called, The only time calc.__del__ would come up is if someone forcefully installed a version of ASE older than what is specified as required. The minimum required version of ASE already removed __del__ from the LAMMPSlib, it only has clean.
calc.clean() covers this too
test for timeout failure results test for fallback
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 `@kim_tools/ase/core.py`:
- Around line 2134-2142: The failure result currently reports the requested
ncells_per_side instead of the generated cell count. Update the result-building
logic around the success and failure paths to use good_ncells on success and
reference_config["ncells_per_side"] on failure, while preserving the existing
reference configuration data. Add an assertion in the timeout test verifying the
top-level cell count matches the failure reference configuration.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 34830ec1-f401-46c2-b513-1a9bd141bb4b
📒 Files selected for processing (2)
kim_tools/ase/core.pytests/test_ase.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_ase.py
| model_name=model_name, | ||
| species_list=[species], | ||
| configuration_type="mono_species_fcc", | ||
| energy_bound=energy_bound, | ||
| coarse_del_a=coarse_del_a, | ||
| coarse_timeout=coarse_timeout, | ||
| nelder_mead_timeout=nelder_mead_timeout, | ||
| max_starting_points=max_starting_points, | ||
| ncells_per_side=ncells_per_side, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Report the generated cell count in failure results.
The reference builder can increase the cell size to fit all species. Line 2187 still returns the requested ncells_per_side when equilibration fails. A timed-out five-species run can therefore report 1 at the top level while mixed_species_result["reference_config"]["ncells_per_side"] reports 2.
Derive the top-level value from good_ncells on success and from reference_config["ncells_per_side"] on failure. Add the same assertion to a timeout test.
Proposed fix
+ actual_ncells_per_side = int(ncells_per_side)
+ if final_result is not None:
+ if final_result.get("ok", False):
+ actual_ncells_per_side = int(final_result["good_ncells"])
+ else:
+ actual_ncells_per_side = int(
+ final_result["reference_config"]["ncells_per_side"]
+ )
+
full_result = {
- "ncells_per_side": ncells_per_side,
+ "ncells_per_side": actual_ncells_per_side,Also applies to: 2160-2168
🤖 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 `@kim_tools/ase/core.py` around lines 2134 - 2142, The failure result currently
reports the requested ncells_per_side instead of the generated cell count.
Update the result-building logic around the success and failure paths to use
good_ncells on success and reference_config["ncells_per_side"] on failure, while
preserving the existing reference configuration data. Add an assertion in the
timeout test verifying the top-level cell count matches the failure reference
configuration.
2 participants
Finding an equilibrium configuration for TorchML models tends to fails because TorchML models have negative infinite energy at 0 distance. This tricks Nelder-Mead optimization routine to crash into a high-density region.
A workaround for this is to do the lattice-sweep in reverse starting from low-density to high-density configuration.
To circumvent the issue of segfaults in high-density configuraitons, the lattice-sweep has to be performed in a new subprocess.
Pending discussion - how to construct configurations for universal models?
NOTE:
This PR was opened to address the improvements required for
find_working_configThe previous PR was closed because the methodology is conceptually different.
#41