Skip to content

Fix relaxation termination summary status - #8226

Open
19hello wants to merge 11 commits into
deepmodeling:developfrom
19hello:fix/issue8087
Open

19hello wants to merge 11 commits into
deepmodeling:developfrom
19hello:fix/issue8087

Conversation

@19hello

@19hello 19hello commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator

Reminder

  • I have read AGENTS.md and docs/developers_guide/agent_governance.md.
  • I have linked an issue or explained why this PR does not need one.
  • I have added adequate unit tests and/or case tests, or explained why not.
  • I have listed the exact verification commands run and their results.
  • I have described user-visible behavior changes, including INPUT parameter changes.
  • I have explained core-module impact for ESolver, HSolver, ElecState, Hamilt, Operator, Psi, or other source/ changes.
  • I have requested any needed governance exception below.

Linked Issue

Fix #8087

Unit Tests and/or Case Tests for my changes

  • Commands run:
cmake -S . -B /tmp/abacus-issue8087-build \
  -DENABLE_MPI=ON \
  -DENABLE_LCAO=OFF \
  -DBUILD_TESTING=ON \
  -DENABLE_JSON=OFF \
  -DCMAKE_BUILD_TYPE=Debug

CCACHE_DISABLE=1 cmake --build /tmp/abacus-issue8087-build \
  --target relax MODULE_RELAX_relax_nsync_test MODULE_RELAX_relax_sync_test -j4

LD_PRELOAD=/usr/lib/x86_64-linux-gnu/openblas-pthread/libopenblas.so.0 \
ctest --test-dir /tmp/abacus-issue8087-build \
  -R 'MODULE_RELAX_relax_(sync|nsync)_test' \
  --output-on-failure

python3 tools/03_code_analysis/code_quality_score.py \
  source/source_relax/relax_driver.cpp \
  source/source_relax/relax_driver.h \
  source/source_relax/relax_nsync.cpp \
  source/source_relax/unittests/test_relax_nsync.cpp \
  source/source_relax/unittests/test_relax_sync.cpp

python3 tools/03_code_analysis/agent_governance_check.py --staged
git diff --cached --check

- Result summary:
  - MODULE_RELAX_relax_sync_test: passed.
  - MODULE_RELAX_relax_nsync_test: passed.
  - relax object target: built successfully.
  - Code quality score: all 5 changed C++ files passed.
  - Governance check: no blocking findings.
  - git diff --check: passed.
- Checks not run, with reason:
  - A complete abacus_pw_para build was started but did not finish within the available execution window.
  - No standalone driver unit target was added because linking relax_driver.cpp directly pulled in the full IO/Cell dependency closure and was not suitable as a lightweight unit-test target.

### What's changed?

- Added explicit relaxation stop-state handling for:
  - genuine geometric convergence;
  - maximum ionic-step termination;
  - user-requested EXIT;
  - dry-run completion;
  - non-relaxation calculations.
- The driver now reports the actual number of evaluated ionic steps, fixing first-step summaries that previously reported zero steps.
- Maximum-step termination is handled only by Relax_Driver.
- IonCellOptimizer now evaluates convergence on the final allowed step before the driver reports relax_nmax termination.
- Updated relaxation unit tests to cover final-step convergence and final-step non-convergence for the non-simultaneous path, while retaining simultaneous-path regression coverage.
- No INPUT parameters were added or changed.

### Governance Notes

- INPUT/docs changes: No documentation update is required. This is an internal correction to relaxation termination reporting and does not change INPUT syntax or parameter semantics.
- Core module impact: No impact on ESolver, HSolver, ElecState, Hamilt, Operator, or Psi. Changes are limited to source_relax driver/optimizer termination handling and its tests.
- Exceptions requested: None.

@ABACUS-PR-BOT ABACUS-PR-BOT left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The terminal-step path can still mix geometry and evaluated properties, and zero-force bfgs_trad needs handling before step construction. Static source review only; builds, runtime tests, and CI results were not verified.

Comment thread source/source_relax/relax_nsync.cpp
Comment thread source/source_relax/relax_driver.cpp
Comment thread source/source_relax/relax_nsync.cpp
Comment thread source/source_relax/unittests/test_relax_nsync.cpp
@mohanchen mohanchen added Bugs Bugs that only solvable with sufficient knowledge of DFT Refactor Refactor ABACUS codes GeometryRelaxation Issues related to geometry relaxation labels Oct 29, 2026

@ABACUS-PR-BOT ABACUS-PR-BOT left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CG validity check and RTD executable path need correction. The earlier JSON frame mismatch and LBFGS/BFGS early-convergence paths are fixed in the inspected source.

Static source review at 4d5cd578ceb67920d63879c7bdbc6d77803caa28; builds, runtime tests, and CI were not independently verified.

Comment on lines +71 to 73
if (ucell.ionic_position_updated || ucell.cell_parameter_updated)
{
geometry_evaluated = false;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the default relax_method=cg, these flags remain set after an earlier Relax::move_cell_ions() call; the later converged return does not clear them. That evaluated frame is then marked unevaluated, so final structure output loses its computed energy, stress, and forces. Reset the flags for each optimizer step after the solver consumes prior changes, and cover a move-then-converge sequence.

Comment thread .readthedocs.yaml
- install/bin/abacus --generate-parameters-yaml > docs/parameters.yaml
- cmake -Bbuild -GNinja -DENABLE_MPI=OFF -DENABLE_LCAO=OFF -DBUILD_TESTING=OFF
- cmake --build build --target abacus_input_doc -j $(nproc)
- build/abacus_input_doc > docs/parameters.yaml

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

abacus_input_doc is declared in source/CMakeLists.txt without an output-directory override, so this Ninja build places it at build/source/abacus_input_doc. This command will fail before parameter generation. Use the actual path or give the new target an explicit runtime output directory.

#include "source_base/matrix.h"
#include "source_base/matrix3.h"
#include "source_cell/unitcell.h"
#include "source_relax/relax_criteria.h"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: this header only takes Relax_Criteria by const reference. Forward-declare the struct here and include its definition in the implementation and tests that construct it.

Comment on lines +29 to +32
MPI_Initialized(&initialized);
if (!initialized)
{
MPI_Init(nullptr, nullptr);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: this fixture adds direct MPI lifecycle calls despite the repository wrapper rule. Use the existing Parallel_Global test-main pattern, including initialization of unused communicators before finalization, or request a scoped test-harness exception.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bugs Bugs that only solvable with sufficient knowledge of DFT GeometryRelaxation Issues related to geometry relaxation Refactor Refactor ABACUS codes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Relaxation final summary can report convergence after EXIT or an iteration-limit stop

3 participants