Skip to content

Refactor OFDFT line-search status to std::string - #7985

Merged
mohanchen merged 2 commits into
deepmodeling:developfrom
zzlinpku:fix/ofdft-task-array
Sep 18, 2026
Merged

mohanchen merged 2 commits into
deepmodeling:developfrom
zzlinpku:fix/ofdft-task-array

Conversation

@zzlinpku

@zzlinpku zzlinpku commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

What's changed?

The OFDFT line-search status was stored in a manually allocated 60-character buffer. Use std::string for ESolver_OF::task_ and pass it by reference through Opt_DCsrch::dcSrch and the internal dcsrch function. Status writes now use string assignment; prefix checks use C++11-compatible compare. This removes manual allocation, fixed-capacity status storage, and raw character writes while preserving status text and numerical logic.

Update both existing CG/TN test callers. Add three focused line-search tests for convergence/restart, error-to-short-status replacement, and a maximum-step warning; register them as MODULE_BASE_opt_dcsrch.

Linked Issue

No linked issue: self-contained ownership and status-string cleanup.

Unit Tests and/or Case Tests for my changes

Verified on the branch based on 3d200fd47b67275d2a101ea036b2634d29c29fb9. Local submitted files were SHA-256 matched to the server files.

Server build configuration (unchanged): ENABLE_LCAO=OFF, ENABLE_LIBXC=OFF, ENABLE_LIBRI=OFF, ENABLE_ELPA=OFF.

Commands run:

git diff --cached --check
python3 tools/03_code_analysis/agent_governance_check.py --staged
cmake --build /home/zzl/build-ofdft-pr-latest -j4

# In a scratch copy of tests/07_OFDFT/03_OF_OP_TN,
# preserving its relative path to tests/PP_ORB:
OMP_NUM_THREADS=1 mpirun --bind-to none -n 2 \
  /home/zzl/build-ofdft-pr-latest/abacus_pw_para

Results:

  • PW build succeeded after the string/interface changes.
  • OFDFT case completed through TN7. Final energy -57.93385514279088 eV, identical to the prior array version; reference -57.93385514279199 eV; absolute difference about 1.108e-12 eV, below 1e-8 eV.
  • MPI emitted the same hwloc PCI-domain/display-authorization warnings; calculation completed successfully.
  • Diff check passed; governance check found no blocking errors.

Focused tests were also compiled locally with Clang, C++11 and GoogleTest 1.12.1:

clang++ -std=c++11 -pthread \
  -I/private/tmp/ofdft-googletest-1121/googletest/include \
  -I/private/tmp/ofdft-googletest-1121/googletest \
  source/source_base/test/test_opt_dcsrch.cpp \
  source/source_base/opt_dcsrch.cpp \
  /private/tmp/ofdft-googletest-1121/googletest/src/gtest-all.cc \
  /private/tmp/ofdft-googletest-1121/googletest/src/gtest_main.cc \
  -o /private/tmp/ofdft-test-opt-dcsrch
/private/tmp/ofdft-test-opt-dcsrch

All 3 tests passed.

Limits:

  • Full regression suite and existing CG/TN test executables were not run locally; their callers were migrated and are left for CI.
  • The new test passed as a standalone executable; its CTest registration has not been exercised locally.
  • Full LCAO build was previously blocked by missing RI/global/Array_Operator.h; only the stated PW build is claimed.
  • Upstream has advanced beyond the branch base. No fresh build of the merge with upstream was performed.

Governance Notes

  • INPUT/docs changes: no INPUT or user-facing calculation behavior changes, so no INPUT documentation update is required. The internal status-parameter documentation is updated.
  • Core module impact: Opt_DCsrch::dcSrch now requires std::string& rather than char*; all in-repository callers are migrated. Out-of-tree callers must adapt. Numerical line-search operations are unchanged.
  • Headers: <string> is required for the owned member and the string-reference interface.
  • Global dependencies: existing diagnostic output is retained; no net increase of global references.
  • Exceptions requested: none.

Reminder

  • Read repository governance.
  • Explained issue linkage, test evidence and limitations.
  • Listed verification commands and results.
  • Described interface, core-module and INPUT impact.
  • Explained header dependencies and remaining global usage.

Copilot AI lite review requested due to automatic review settings September 17, 2026 09:53

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

No unresolved review issues remain, and existing behavior is preserved.

Pull request overview

Refactors the OFDFT line-search buffer to use RAII-managed std::array storage while preserving behavior.

Changes:

  • Replaces manual allocation with std::array<char, 60>.
  • Removes matching allocation and deallocation.
  • Updates buffer usage to call .data().
File summaries
File Description
source/source_esolver/esolver_of.h Declares the fixed-size array buffer.
source/source_esolver/esolver_of.cpp Removes manual buffer management.
source/source_esolver/esolver_of_interface.cpp Uses the array buffer through .data().
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread source/source_esolver/esolver_of.h Outdated
@mohanchen mohanchen added the Refactor Refactor ABACUS codes label Sep 17, 2026
@zzlinpku zzlinpku changed the title Refactor OFDFT line-search task buffer to std::array Refactor OFDFT line-search status to std::string Sep 18, 2026
@mohanchen
mohanchen merged commit a4506f3 into deepmodeling:develop Sep 18, 2026
18 checks passed
@mohanchen

Copy link
Copy Markdown
Collaborator

Nice try as a first PR, please keep going. @zzlinpku

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

Labels

Refactor Refactor ABACUS codes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants