Skip to content

Refactor(input): make relax_method a scalar option - #8168

Open
Stardust0831 wants to merge 2 commits into
deepmodeling:developfrom
Stardust0831:fix/7822-relax-method-scalar
Open

Stardust0831 wants to merge 2 commits into
deepmodeling:developfrom
Stardust0831:fix/7822-relax-method-scalar

Conversation

@Stardust0831

@Stardust0831 Stardust0831 commented Oct 17, 2026 •

Copy link
Copy Markdown
Collaborator

relax_method now selects each relaxation method with one scalar string. This removes the algorithm/variant pair from INPUT parsing and the relaxation interfaces, makes availability expressions refer to one well-defined value, and gives invalid inputs a complete list of accepted choices.

The relax_method values change as follows:

Previous value New value
cg or cg 2 cg
cg 1 cg_trad
bfgs or bfgs 2 bfgs
bfgs 1 bfgs_trad
cg_bfgs cg_bfgs (unchanged)
lbfgs lbfgs (unchanged)
sd sd (unchanged)

The default remains simultaneous CG (cg). All two-token values are rejected; no compatibility aliases are retained.

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 (none needed).

Linked Issue

Fixes #7822. Also addresses the availability-semantics discussion in #7783.

That discussion identified ambiguity between scalar membership (in) and vector containment (contains): when relax_method is a vector, it is unclear whether a condition refers to the whole vector, its first element, or one of its elements. A scalar parameter removes that ambiguity. The existing availability grammar remains unchanged.

Unit Tests and/or Case Tests for my changes

  • Passed: PW/MPI build, 76 unit tests across seven CTest targets, and four-rank scalar broadcast.
  • CLI checks accepted seven valid options and rejected nine invalid inputs with all choices listed. Help and generated documentation were verified.
  • Whitespace and governance checks passed.

Not run: full trajectory comparisons, LCAO/GPU builds, or the full test suite; validation focused on PW/MPI. The broader MPI ParaRead fixture requires ELPA.

Verification commands and build details

GCC 13.4, PW/MPI, LCAO/ELPA disabled, assertions enabled, OMP_NUM_THREADS=1; executable version v3.11.0-beta10. $ABACUS_BUILD_DIR is the build directory; $PARAMETER_YAML and $INPUT_REFERENCE are generated scratch files. CLI input checks used copies of tests/01_PW/060_PW_RE_MG.

cmake --build "$ABACUS_BUILD_DIR" -j 8 --target MODULE_IO_read_input_serial MODULE_IO_read_item_serial MODULE_IO_availability MODULE_RELAX_ions_move_methods_test MODULE_RELAX_ions_move_cg_test MODULE_RELAX_relax_nsync_test MODULE_RELAX_relax_sync_test MODULE_IO_input_test_para abacus_pw_para
ctest --test-dir "$ABACUS_BUILD_DIR" --output-on-failure -R '^(MODULE_IO_(read_input_serial|read_item_serial|availability)|MODULE_RELAX_(ions_move_methods|ions_move_cg|relax_nsync|relax_sync)_test)$'
GTEST_FILTER=InputParaTest.TypedTDFieldLists ctest --test-dir "$ABACUS_BUILD_DIR" --timeout 60 --output-on-failure -R '^MODULE_IO_input_test_para_4$'
"$ABACUS_BUILD_DIR/abacus_pw_para" --version
"$ABACUS_BUILD_DIR/abacus_pw_para" -h relax_method
"$ABACUS_BUILD_DIR/abacus_pw_para" --check-input
"$ABACUS_BUILD_DIR/abacus_pw_para" --generate-parameters-yaml > "$PARAMETER_YAML"
python docs/generate_input_main.py "$PARAMETER_YAML" --output "$INPUT_REFERENCE"
git diff --check upstream/develop...HEAD
python3 tools/03_code_analysis/agent_governance_check.py --base upstream/develop --head HEAD --format text
git diff --name-only upstream/develop...HEAD | rg '\.(cpp|h)$' | xargs python3 tools/03_code_analysis/code_quality_score.py

Quality scoring: 15/17 files passed; both existing low scores are unchanged from the base.

What's changed?

  • Store and pass relax_method as one std::string. Accept exactly one of cg (default), cg_trad, bfgs, bfgs_trad, cg_bfgs, lbfgs, or sd; reject empty, unknown, and multi-token values without compatibility aliases.
  • Simplify parsing, MPI synchronization, dispatch, and reporting to use scalar names. Preserve the existing numerical algorithms, CG-to-traditional-BFGS transition, and BFGS history resets after cell changes.
  • Update Input_Item type/default/help and dependent availability expressions. Invalid empty, unknown, and multi-token values now share one diagnostic that lists every accepted scalar choice. Add assertions that scalar membership is valid and vector containment is rejected for relax_method.
  • Update the geometry-optimization manual and availability-guide examples to use scalar option names. Remove obsolete spellings, numbered variants, and migration prose from the manual and CLI help.
  • Correct example and regression-test INPUT files, test fixtures, and their comments to use current names. Retain invalid legacy input only in rejection tests.

The diff contains 40 files: 9 implementation files, 10 unit-test/fixture files, 2 documentation files, and 19 example/regression INPUT files. Each example/regression INPUT change replaces one relaxation selection.

Governance Notes

  • INPUT/docs changes: relax_method is now scalar throughout. Input_Item metadata is the source of truth; generated YAML and Markdown references were checked but are not committed.
  • Core module impact: changes are confined to IO representation and relaxation selection/reporting. No numerical kernels or new global dependencies are introduced.
  • Historical quality debt: two existing large IO test files remain below the quality-score threshold, with unchanged scores relative to the base. No new governance findings; no exceptions requested.

@Stardust0831
Stardust0831 marked this pull request as ready for review October 17, 2026 16:12
@mohanchen mohanchen added GeometryRelaxation Issues related to geometry relaxation Refactor Refactor ABACUS codes labels Oct 17, 2026
@mohanchen

Copy link
Copy Markdown
Collaborator

Thanks! Maybe it would be better to change 'cg_nested' to 'cg_trad' and change 'bfgs_traditional' to 'bfgs_trad‘?

@mohanchen

Copy link
Copy Markdown
Collaborator

@ABACUS-PR-BOT Could you review the PR?

@mohanchen
mohanchen self-requested a review October 17, 2026 22:31

@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.

Static review of 96cca9ce281ce38512c91ec5812f7ac1cbb1baf2: no new correctness issue found in the scalar parsing and relaxation dispatch changes. One optional documentation suggestion below. No build/runtime tests were run; CI diagnostics were unavailable through the required checks connector.

Comment thread docs/advanced/opt.md
## Optimization Algorithms

ABACUS offers multiple optimization algorithms for structural relaxation, which can be selected using the [relax_method](./input_files/input-main.md#relax_method) keyword. The optional second value selects an implementation variant for CG or BFGS. For both methods, variant 1 is the traditional implementation and variant 2 is the recommended default.
ABACUS offers multiple optimization algorithms for structural relaxation, which can be selected using the [relax_method](./input_files/input-main.md#relax_method) keyword. Specify one scalar value.

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: consider adding a short upgrade table here for the finalized scalar names (currently cg 1 -> cg_nested, cg 2 -> cg, bfgs 1 -> bfgs_traditional, bfgs 2 -> bfgs). The old two-token inputs now fail validation, and this mapping is only in the PR description. A documentation follow-up after merge is fine.

@Stardust0831

Copy link
Copy Markdown
Collaborator Author

@mohanchen Thank you for the review and suggestions; I’ve renamed the options to cg_trad and bfgs_trad as requested in a follow-up commit.

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

GeometryRelaxation Issues related to geometry relaxation Refactor Refactor ABACUS codes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Normalize relax_method as a scalar option and deprecate two-token values

3 participants