feature/SOF-8009 Feat: AFIR - #356
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
📝 WalkthroughWalkthroughAdds an MLFF reaction-path discovery category and a complete AFIR/MACE Claisen rearrangement notebook. The workflow prepares structures, searches and refines the reaction path, validates the transition state, stores materials, and exports analysis results. ChangesAFIR/MACE reaction path discovery
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🟠 High · up to The AFIR workflow can currently generate incorrect reaction paths by accepting invalid atom selections, continuing after failed optimization stages, using an invalid transition mode, or misidentifying endpoint minima. These correctness risks can lead to misleading exported results and should be fixed before merging. Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant User
participant PubChem
participant MACE
participant ASE
participant MaterialsStorage
User->>PubChem: Fetch reactant if no local upload exists
PubChem-->>User: Return molecular structure
User->>MACE: Configure calculator
MACE->>ASE: Evaluate energies and forces
ASE-->>User: Return relaxed structures and reaction path
User->>MaterialsStorage: Save reactant, transition state, and product metadata
MaterialsStorage-->>User: Return stored materials and exported results
🚥 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: 2
🧹 Nitpick comments (2)
other/materials_designer/workflows/local/reaction_path_afir_mace.ipynb (2)
486-490: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winClear the vibration cache before the run.
Vibrationsstores each displacement result in thetransition_state_vibrationscache folder and reuses any file it finds.vibrations.clean()runs only at the end of this cell. If an earlier run stopped betweenrun()andclean(), the next run reuses results computed for a different geometry, and the reported frequencies are wrong without any error.🛠️ Proposed fix
vibrations = Vibrations(transition_state, name="transition_state_vibrations") +vibrations.clean() vibrations.run()🤖 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/local/reaction_path_afir_mace.ipynb` around lines 486 - 490, Update the vibration setup around the Vibrations instance to clear the existing transition_state_vibrations cache before calling vibrations.run(), ensuring stale displacement results are not reused while preserving the existing summary flow.
285-300: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClose the trajectory writer before you read the file.
trajectorystays open for the rest of the notebook. Cell 5.1 readsAFIR_TRAJECTORY_PATHwhile the writer still holds it. On the Emscripten filesystem used by JupyterLite, unflushed frames can make the reconstructed path shorter than the search actually produced. Close the writer at the end of the ramp.🛠️ Proposed fix
-structure.set_constraint() +structure.set_constraint() +trajectory.close()🤖 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/local/reaction_path_afir_mace.ipynb` around lines 285 - 300, Close the trajectory writer after the AFIR_FORCE_RAMP loop completes and before any later cell reads AFIR_TRAJECTORY_PATH. Add the close operation immediately after the final structure.set_constraint() call, using the existing trajectory object created as Trajectory.
🤖 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/local/reaction_path_afir_mace.ipynb`:
- Around line 161-167: Ensure FOLDER is created before the molecule_path
existence check and fetched-structure write, so both molecule and materials
outputs can use it on the first run. Update the PubChem request in
fetch_pubchem_structure to call quote with safe="" and configure urlopen with an
explicit timeout.
- Around line 493-503: Guard the imaginary_mode_indices lookup after its
comprehension in the cell at
other/materials_designer/workflows/local/reaction_path_afir_mace.ipynb:493-503
by raising a clear error when no qualifying modes exist, stating that the
transition state was not confirmed and that SADDLE_FMAX, the dimer step limit,
or IMAGINARY_MODE_THRESHOLD may need adjustment; the later reuse at
other/materials_designer/workflows/local/reaction_path_afir_mace.ipynb:646-651
requires no direct change because the earlier cell now fails fast.
---
Nitpick comments:
In `@other/materials_designer/workflows/local/reaction_path_afir_mace.ipynb`:
- Around line 486-490: Update the vibration setup around the Vibrations instance
to clear the existing transition_state_vibrations cache before calling
vibrations.run(), ensuring stale displacement results are not reused while
preserving the existing summary flow.
- Around line 285-300: Close the trajectory writer after the AFIR_FORCE_RAMP
loop completes and before any later cell reads AFIR_TRAJECTORY_PATH. Add the
close operation immediately after the final structure.set_constraint() call,
using the existing trajectory object created as Trajectory.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 94f5cd18-6ec3-45b3-8d07-d4f0e27fb4fd
📒 Files selected for processing (2)
other/materials_designer/workflows/Introduction.ipynbother/materials_designer/workflows/local/reaction_path_afir_mace.ipynb
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
other/materials_designer/workflows/local/reaction_path_afir_mace.ipynb (2)
559-560: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUse one complete validity gate before exporting a transition state.
The workflow continues after an unconverged dimer. It substitutes mode
0when no reaction mode exists. It accepts multiple qualifying imaginary modes by selecting the first. It also accepts any two distinct relaxed structures without confirming that they match the reactant and product.Compute one
transition_state_validvalue only after all checks pass: converged saddle, exactly one qualifying imaginary mode, converged endpoint relaxations, and endpoints that map to the relaxed reactant and product. Export a transition-state material only when this value isTrue. Otherwise, label the structure as a transition-state candidate.
other/materials_designer/workflows/local/reaction_path_afir_mace.ipynb#L559-L560: prevent downstream transition-state processing after a non-converged saddle.other/materials_designer/workflows/local/reaction_path_afir_mace.ipynb#L592-L605: require exactly one qualifying imaginary mode before selectingreaction_mode.other/materials_designer/workflows/local/reaction_path_afir_mace.ipynb#L629-L651: verify both endpoint optimizations and match the endpoints toreactantandproduct.other/materials_designer/workflows/local/reaction_path_afir_mace.ipynb#L743-L751: do not export an invalid candidate with thetransition statename.other/materials_designer/workflows/local/reaction_path_afir_mace.ipynb#L781-L786: serialize the complete validity result.Also applies to: 592-605, 629-651, 743-751, 781-786
🤖 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/local/reaction_path_afir_mace.ipynb` around lines 559 - 560, Update the reaction-path workflow to compute a single transition_state_valid result only after confirming a converged saddle, exactly one qualifying imaginary mode, converged endpoint relaxations, and endpoints matching the relaxed reactant and product; remove fallback mode selection and first-match acceptance. Gate transition-state export and naming on this value, labeling invalid results as transition-state candidates, and serialize the complete validity result.
351-351: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSet a finite step limit and require convergence for both relaxations.
ASE defaults
BFGS.runto 100,000,000 steps. Store each return value and stop the workflow when either isFalse. Pass a sharedRELAXATION_MAX_STEPSto both calls.🤖 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/local/reaction_path_afir_mace.ipynb` at line 351, Update both relaxation calls using BFGS.run to pass the shared RELAXATION_MAX_STEPS limit and capture each return value. After both relaxations, stop the workflow if either result is False, while preserving the existing fmax and logfile settings.
🤖 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.
Outside diff comments:
In `@other/materials_designer/workflows/local/reaction_path_afir_mace.ipynb`:
- Around line 559-560: Update the reaction-path workflow to compute a single
transition_state_valid result only after confirming a converged saddle, exactly
one qualifying imaginary mode, converged endpoint relaxations, and endpoints
matching the relaxed reactant and product; remove fallback mode selection and
first-match acceptance. Gate transition-state export and naming on this value,
labeling invalid results as transition-state candidates, and serialize the
complete validity result.
- Line 351: Update both relaxation calls using BFGS.run to pass the shared
RELAXATION_MAX_STEPS limit and capture each return value. After both
relaxations, stop the workflow if either result is False, while preserving the
existing fmax and logfile settings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 80ab3414-e525-4712-b07a-1ebb23c00339
📒 Files selected for processing (1)
other/materials_designer/workflows/local/reaction_path_afir_mace.ipynb
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 (2)
other/materials_designer/workflows/local/reaction_path_afir_mace.ipynb (2)
625-634: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftVerify that the two endpoints match the intended minima.
connects_two_minimaonly proves that the forward and reverse relaxations differ in one tracked distance. Two unrelated minima can satisfy that test. The notebook then setstransition_state_foundtoTrueeven when neither endpoint matches the optimized reactant and product.Compare both relaxed endpoints with
reactantandproduct, accept either direction ordering, and settransition_state_foundonly when both matches succeed. Otherwise report the result as an unassigned saddle connection.Also applies to: 768-768
🤖 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/local/reaction_path_afir_mace.ipynb` around lines 625 - 634, Update the endpoint-validation logic around connects_two_minima and transition_state_found to compare both relaxed endpoints against reactant and product using the existing distance-matching mechanism. Accept either forward/reactant with reverse/product or the opposite ordering, and set transition_state_found only when one complete pairing matches; otherwise report the saddle connection as unassigned.
577-590: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRequire exactly one reaction mode before mode following.
When no qualifying imaginary mode exists, Line 590 uses mode 0 and continues with a non-reaction mode. When multiple modes exist, it silently selects the first one. Both cases contradict the first-order-saddle requirement and can produce invalid connected minima and exported results. Stop unless exactly one mode passes the threshold.
Proposed fix
- if not imaginary_mode_indices: - print("⚠️ No imaginary mode above the threshold — this structure is not a transition state.") - - reaction_mode = vibrations.get_mode(imaginary_mode_indices[0] if imaginary_mode_indices else 0) + if len(imaginary_mode_indices) != 1: + raise RuntimeError( + "Transition state validation failed: expected exactly one imaginary mode above " + f"{IMAGINARY_MODE_THRESHOLD} cm⁻¹, found {len(imaginary_mode_indices)}." + ) + + reaction_mode = vibrations.get_mode(imaginary_mode_indices[0])🤖 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/local/reaction_path_afir_mace.ipynb` around lines 577 - 590, Update the imaginary-mode validation around imaginary_mode_indices and reaction_mode so execution stops unless exactly one mode exceeds IMAGINARY_MODE_THRESHOLD. Remove the fallback to mode 0, select the sole qualifying index only after validation, and prevent subsequent mode-following, minima generation, or export logic from running for zero or multiple qualifying modes.
🤖 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/local/reaction_path_afir_mace.ipynb`:
- Around line 232-237: Update the TRACKED_PAIRS validation that computes
out_of_range to also reject negative atom indices and pairs whose two atom
indices are identical, while preserving the existing upper-bound and
error-reporting behavior. Ensure invalid pairs are reported before molecule
indexing or reaction-direction processing.
- Around line 383-384: Update the AFIR force-ramping loop around BFGS.run(...)
to capture its convergence boolean and stop immediately when a stage fails to
converge within AFIR_MAX_STEPS_PER_STAGE. Raise a clear error identifying the
failed force stage instead of passing its final geometry to the next stage;
retain the existing progression for converged stages.
---
Outside diff comments:
In `@other/materials_designer/workflows/local/reaction_path_afir_mace.ipynb`:
- Around line 625-634: Update the endpoint-validation logic around
connects_two_minima and transition_state_found to compare both relaxed endpoints
against reactant and product using the existing distance-matching mechanism.
Accept either forward/reactant with reverse/product or the opposite ordering,
and set transition_state_found only when one complete pairing matches; otherwise
report the saddle connection as unassigned.
- Around line 577-590: Update the imaginary-mode validation around
imaginary_mode_indices and reaction_mode so execution stops unless exactly one
mode exceeds IMAGINARY_MODE_THRESHOLD. Remove the fallback to mode 0, select the
sole qualifying index only after validation, and prevent subsequent
mode-following, minima generation, or export logic from running for zero or
multiple qualifying modes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 452d47e5-7cfc-4273-9306-532c32c74040
📒 Files selected for processing (1)
other/materials_designer/workflows/local/reaction_path_afir_mace.ipynb
| "out_of_range = {label: pair for label, pair in TRACKED_PAIRS.items() if max(pair) >= len(molecule)}\n", | ||
| "if out_of_range:\n", | ||
| " raise ValueError(\n", | ||
| " f\"{out_of_range} out of range for {MOLECULE_NAME}, which has {len(molecule)} atoms. \"\n", | ||
| " \"Set the pairs in 1.2 from the listing above.\"\n", | ||
| " )\n", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject negative and duplicate atom indices.
Line 232 only rejects indices above the atom count. Python accepts negative indices, so -1 silently selects the last atom and can search a different reaction. A pair with the same atom twice also creates a zero-length reaction direction later.
Proposed fix
- out_of_range = {label: pair for label, pair in TRACKED_PAIRS.items() if max(pair) >= len(molecule)}
- if out_of_range:
+ invalid_pairs = {
+ label: pair
+ for label, pair in TRACKED_PAIRS.items()
+ if min(pair) < 0 or max(pair) >= len(molecule) or pair[0] == pair[1]
+ }
+ if invalid_pairs:
raise ValueError(
- f"{out_of_range} out of range for {MOLECULE_NAME}, which has {len(molecule)} atoms. "
- "Set the pairs in 1.2 from the listing above."
+ f"{invalid_pairs} contains invalid atom pairs for {MOLECULE_NAME}, which has {len(molecule)} atoms. "
+ "Use distinct, non-negative indices from the listing in 2.1."
)📝 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.
| "out_of_range = {label: pair for label, pair in TRACKED_PAIRS.items() if max(pair) >= len(molecule)}\n", | |
| "if out_of_range:\n", | |
| " raise ValueError(\n", | |
| " f\"{out_of_range} out of range for {MOLECULE_NAME}, which has {len(molecule)} atoms. \"\n", | |
| " \"Set the pairs in 1.2 from the listing above.\"\n", | |
| " )\n", | |
| "invalid_pairs = {\n", | |
| " label: pair\n", | |
| " for label, pair in TRACKED_PAIRS.items()\n", | |
| " if min(pair) < 0 or max(pair) >= len(molecule) or pair[0] == pair[1]\n", | |
| "}\n", | |
| "if invalid_pairs:\n", | |
| " raise ValueError(\n", | |
| " f\"{invalid_pairs} contains invalid atom pairs for {MOLECULE_NAME}, which has {len(molecule)} atoms. \"\n", | |
| " \"Use distinct, non-negative indices from the listing in 2.1.\"\n", | |
| " )\n", |
🤖 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/local/reaction_path_afir_mace.ipynb`
around lines 232 - 237, Update the TRACKED_PAIRS validation that computes
out_of_range to also reject negative atom indices and pairs whose two atom
indices are identical, while preserving the existing upper-bound and
error-reporting behavior. Ensure invalid pairs are reported before molecule
indexing or reaction-direction processing.
| " distances = \", \".join(f\"{label} = {structure.get_distance(*pair):.2f} Å\" for label, pair in TRACKED_PAIRS.items())\n", | ||
| " print(f\"α = {artificial_force:.1f} eV/Å → {distances}\")\n", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
file="other/materials_designer/workflows/local/reaction_path_afir_mace.ipynb"
python3 - <<'PY'
import json
from pathlib import Path
p = Path("other/materials_designer/workflows/local/reaction_path_afir_mace.ipynb")
nb = json.loads(p.read_text())
for i, cell in enumerate(nb.get("cells", [])):
text = "".join(cell.get("source", []))
if "AFIR_MAX_STEPS_PER_STAGE" in text or "distances = " in text or "artificial_force" in text:
print(f"\n--- cell {i} ({cell.get('cell_type')}) ---")
for n, line in enumerate(text.splitlines(), 1):
print(f"{n:4}: {line}")
PYRepository: mat3ra/api-examples
Length of output: 7070
🏁 Script executed:
#!/bin/bash
set -e
file="other/materials_designer/workflows/local/reaction_path_afir_mace.ipynb"
test -f "$file"
python3 - "$file" <<'PY'
import json, sys
p = sys.argv[1]
nb = json.load(open(p))
for i, c in enumerate(nb["cells"]):
s = "".join(c.get("source", []))
if any(x in s for x in ("AFIR_MAX_STEPS_PER_STAGE", "BFGS(structure", "distances = ", "artificial_force =")):
print(f"--- cell {i} ---")
print(s)
PYRepository: mat3ra/api-examples
Length of output: 4638
🏁 Script executed:
pwd
git ls-files other/materials_designer/workflows/local/reaction_path_afir_mace.ipynb
wc -l other/materials_designer/workflows/local/reaction_path_afir_mace.ipynb
sed -n '350,410p' other/materials_designer/workflows/local/reaction_path_afir_mace.ipynbRepository: mat3ra/api-examples
Length of output: 2436
🌐 Web query:
ASE Optimizer converged fmax BFGS run steps maximum force get_forces implementation
💡 Result:
In the Atomic Simulation Environment (ASE), structure optimizers like BFGS follow a standardized interface for convergence and execution [1][2]. Convergence Criteria (fmax) The fmax parameter represents the maximum allowed force component on any individual atom [1]. An optimization run is considered converged when the force on every atom is less than fmax: max_a |F_a| < fmax [1][2] In the internal implementation, this is typically evaluated by calculating the norm of the forces for each atom and checking if the maximum value satisfies the criterion [3]. Execution (run steps) The run method controls the optimization process, accepting both fmax and a maximum number of steps as arguments [4][2]. The optimizer will terminate if: 1. The convergence criterion (fmax) is met [4][3]. 2. The number of optimization steps exceeds the specified maximum [4][3]. Implementation Details The base Optimizer class handles the main loop of the optimization, which includes retrieving forces, logging progress, and checking for convergence [3]. - Forces: Calculators attached to the Atoms object provide the forces via the get_forces method [1][5]. - Recent API Changes: ASE has been transitioning its internal API to use a gradient-based approach, where optimizers increasingly rely on get_gradient (which returns a flattened array of the negative forces) rather than directly calling get_forces in all contexts [6]. - Hessian Estimation: Optimizers like BFGS maintain an estimate of the Hessian (curvature of the potential energy surface), which is updated at each step to refine the step direction [1][7][2]. For users, the standard usage is typically: dyn = BFGS(atoms) dyn.run(fmax=0.05, steps=100) [1][4]
Citations:
- 1: https://ase-lib.org/ase/optimize.html
- 2: https://docs.ase-lib.org/ase/optimize.html
- 3: https://github.com/qsnake/ase/blob/master/ase/optimize/optimize.py
- 4: https://ase-lib.org/_modules/ase/optimize/optimize.html
- 5: https://mattermodeling.stackexchange.com/questions/1927/ases-fmax-and-fhi-aimss-relax-geometry-bfgs-tolerance-are-they-same
- 6: https://gitlab.com/ase/ase/-/merge_requests/3732.diff
- 7: https://ase-lib.org/_modules/ase/optimize/bfgs.html
🌐 Web query:
site:ase-lib.org/_modules/ase/optimize/optimize.py "forces**2" "converged"
💡 Result:
The matching ASE source is:
ase.optimize.optimize — specifically the Optimizer.converged() and gradient_converged() methods. The current implementation delegates convergence testing to self.optimizable.converged(...); it does not contain the literal forces**2 expression there. (docs.ase-lib.org)
The force-norm calculation appears in related modules, such as ase.mep.neb and ase.mep.dimer. (docs.ase-lib.org)
Citations:
- 1: https://docs.ase-lib.org/_modules/ase/optimize/optimize.html?utm_source=openai
- 2: https://docs.ase-lib.org/_modules/ase/mep/neb.html?utm_source=openai
Stop the force ramp when an AFIR stage does not converge. The loop ignores the boolean returned by BFGS.run(...) and passes the final geometry to the next force stage. Raise a clear error when the stage reaches AFIR_MAX_STEPS_PER_STAGE without meeting AFIR_FMAX.
🤖 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/local/reaction_path_afir_mace.ipynb`
around lines 383 - 384, Update the AFIR force-ramping loop around BFGS.run(...)
to capture its convergence boolean and stop immediately when a stage fails to
converge within AFIR_MAX_STEPS_PER_STAGE. Raise a clear error identifying the
failed force stage instead of passing its final geometry to the next stage;
retain the existing progression for converged stages.
2 participants
Summary by CodeRabbit