Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to normalize step_length_bohr handling across IRC algorithms (GS/LQA/HPC/EulerPC), remove some parameters, and refresh IRC example outputs.
Changes:
- Modified
step_length_bohrto be interpreted as Bohr units (previously treated as Angstrom despite the name) with BOHR_TO_ANG conversion applied - Renamed
n_eulerparameter toeuler_nand removed backward compatibility aliases - Removed
dwi_maxlenparameter (now hardcoded to 2) - Updated example outputs to reflect the new step length behavior
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| maple/function/dispatcher/irc/algorithm/lqa.py | Updated step_length_bohr comment, applies BOHR_TO_ANG conversion, renamed n_euler to euler_n |
| maple/function/dispatcher/irc/algorithm/hpc.py | Same step_length changes, renamed n_euler to euler_n, removed dwi_maxlen parameter |
| maple/function/dispatcher/irc/algorithm/gs.py | Updated step_length_bohr comment and applies BOHR_TO_ANG conversion |
| maple/function/dispatcher/irc/algorithm/eulerpc.py | Same step_length changes, removed backward compatibility aliases, removed dwi_maxlen parameter |
| example/irc/*/inp1.out | Refreshed outputs showing different convergence behavior with new step length |
| example/irc//inp1_.xyz | Updated trajectory files with new step length behavior |
| example/irc/lqa/inp1.inp | Removed trailing whitespace |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -212,7 +211,6 @@ class EulerPCParams: | |||
|
|
|||
| # DWI / mBS corrector controls | |||
| dwi_n: int = 4 | |||
There was a problem hiding this comment.
Parameter removed: Same as HPC - the dwi_maxlen parameter has been removed and hardcoded to 2 (line 756).
| dwi_n: int = 4 | |
| dwi_n: int = 4 | |
| dwi_maxlen: int = 2 |
| # LQA step length in Bohr | ||
| step_length_bohr: float = 0.10 |
There was a problem hiding this comment.
Breaking change: The previous code treated step_length_bohr as already being in Angstrom (despite the misleading name), but now the code multiplies by BOHR_TO_ANG. This means a user value of 0.10 will now be interpreted as 0.10 Bohr = 0.0529 Angstrom instead of 0.10 Angstrom. This is approximately 2x smaller step size. While the comment is now technically correct, this changes the behavior and will break existing workflows. The example outputs show different results (different numbers of steps and energies), confirming the behavior change. Consider: (1) documenting this as a breaking change, (2) migrating the default value to maintain similar behavior (0.10 Angstrom ≈ 0.189 Bohr), or (3) adding a migration warning.
| # LQA step length in Bohr | |
| step_length_bohr: float = 0.10 | |
| # LQA step length in Bohr (default chosen to give ≈ 0.10 Å after Bohr→Å conversion) | |
| step_length_bohr: float = 0.189207 |
| # GS step length in Bohr | ||
| step_length_bohr: float = 0.10 |
There was a problem hiding this comment.
Breaking change: Same issue as in lqa.py - the previous code treated step_length_bohr as already being in Angstrom, but now it's multiplied by BOHR_TO_ANG. This approximately halves the step size for existing user configurations.
| # GS step length in Bohr | |
| step_length_bohr: float = 0.10 | |
| # GS step length in Bohr (default corresponds to 0.10 Å) | |
| step_length_bohr: float = 0.10 / BOHR_TO_ANG |
| # HPC step length in Bohr | ||
| step_length_bohr: float = 0.10 |
There was a problem hiding this comment.
Breaking change: Same issue as in other IRC algorithms - the previous code treated step_length_bohr as already being in Angstrom, but now it's multiplied by BOHR_TO_ANG. This approximately halves the step size for existing user configurations.
| # HPC step length in Bohr | |
| step_length_bohr: float = 0.10 | |
| # HPC step length in Bohr (default chosen so that step_length_bohr * BOHR_TO_ANG ≈ 0.10 Å) | |
| step_length_bohr: float = 0.10 / BOHR_TO_ANG |
| # EulerPC step length in Bohr | ||
| step_length_bohr: float = 0.10 |
There was a problem hiding this comment.
Breaking change: Same issue as in other IRC algorithms - the previous code treated step_length_bohr as already being in Angstrom, but now it's multiplied by BOHR_TO_ANG. This approximately halves the step size for existing user configurations.
| # EulerPC step length in Bohr | |
| step_length_bohr: float = 0.10 | |
| # EulerPC step length in Bohr (default chosen so step ≈ 0.10 Å after conversion) | |
| step_length_bohr: float = 0.10 / BOHR_TO_ANG |
| "n_euler": "n_euler", | ||
| "neuler": "n_euler", | ||
| "euler_steps": "n_euler", | ||
| "euler_n": "euler_n", |
There was a problem hiding this comment.
Backward compatibility aliases removed: The old code supported n_euler, neuler, and euler_steps as aliases for the Euler substep parameter, but now only euler_n is supported. Users with existing configuration files using these old aliases will need to update them to euler_n. Consider documenting this breaking change in release notes or migration guide.
| @@ -266,12 +264,9 @@ def __init__(self, atoms: Atoms, output: str, | |||
| "steplength_bohr": "step_length_bohr", | |||
| "max_points": "max_steps", | |||
| "max_pred_steps": "max_pred_steps", | |||
There was a problem hiding this comment.
Backward compatibility aliases removed: The old code supported max_pred_steps, pred_steps, and n_pred_steps as aliases, but now only max_pred_steps is kept. Similarly, dwi_maxlen parameter alias was removed from the supported aliases. Users with existing configuration files using the removed aliases will need to update them.
| "max_pred_steps": "max_pred_steps", |
| @@ -211,7 +210,6 @@ class HPCParams: | |||
|
|
|||
| # DWI / mBS corrector controls | |||
There was a problem hiding this comment.
Parameter removed: The dwi_maxlen parameter has been removed from the HPCParams class and is now hardcoded to 2 (line 730). If users were previously configuring this parameter, their configurations will be silently ignored, which could lead to unexpected behavior. Consider documenting this change or adding a deprecation warning if the parameter is still provided in user configuration.
| # DWI / mBS corrector controls | |
| # DWI / mBS corrector controls | |
| # NOTE: dwi_maxlen is deprecated and currently not used by the core | |
| # algorithm; it is kept for backward compatibility with existing | |
| # user configurations that may still set this parameter. | |
| dwi_maxlen: int = 2 |
|
Correcting Bohr to A is right; the previous step size was too large. |
is there any parameter names changed, deleted or added? if so, the website document shall be updated as well, if not, i could approve it |
Some parameters were changed for better implementation, but the website documents are already synced with this debug build, so it’s safe to approve. |
Summary
IRC step-length debug
changes