Skip to content

Unify DFT+U occupation-matrix output across PW and LCAO - #8242

Open
lanshuyue wants to merge 6 commits into
deepmodeling:developfrom
lanshuyue:codex/unify-dftu-ion-step-output
Open

lanshuyue wants to merge 6 commits into
deepmodeling:developfrom
lanshuyue:codex/unify-dftu-ion-step-output

Conversation

@lanshuyue

@lanshuyue lanshuyue commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Prepare per-ionic-step DFT+U occupation-matrix output in shared ESolver_KS::iter_init() for both PW and LCAO, removing duplicate setup calls.
  • Rename the compact reader/writer sources to dftu_base_read.cpp and dftu_base_write.cpp, updating the CMake and legacy Makefile lists. Their contents are unchanged.
  • Let PW write occ_mat.txt at electronic step 1 when a matrix was loaded from file, using the same readiness decision as the numbered snapshot. A fresh PW run still records N/A in its first numbered snapshot.

Verification

  • Rebased onto current develop, which includes the upstream lightweight abacus_input_yaml Read the Docs build target (Docs(RTD): build only the INPUT-registry dumper, not all of ABACUS #8248). The prior documentation build timed out at 900 seconds while compiling the full install target; the new abacus-develop doc build passed in 141 seconds, and the abacus-rtd preview check also passed. No documentation configuration is changed by this PR.
  • cmake -S . -B /tmp/abacus-dftu-rename-build -DBUILD_TESTING=ON -DENABLE_LCAO=ON -DENABLE_MPI=ON -DENABLE_LIBXC=OFF -DENABLE_ELPA=OFF: configured. ELPA was disabled locally because this machine's ELPA headers and library do not link together.
  • cmake --build /tmp/abacus-dftu-rename-build --target abacus_basic_para MODULE_PW_dftu_base_test MODULE_DFTU_base_io -j 8: built.
  • OMP_NUM_THREADS=1 ctest --test-dir /tmp/abacus-dftu-rename-build --output-on-failure -R '^(MODULE_PW_dftu_base_test|MODULE_DFTU_base_io)$': 2/2 passed.
  • cmake --build /tmp/abacus-dftu-rename-build --target abacus_input_yaml -j 8 and generated INPUT documentation from its YAML output: passed, 614 parameters documented.
  • python3 tools/03_code_analysis/agent_governance_check.py --staged: no findings. git diff --cached --check: passed.

Review notes

  • No linked issue: this is a focused DFT+U output cleanup and PW/LCAO alignment.
  • No INPUT parameter or occupation-matrix file-format changes; user defaults are unchanged.
  • No new test is added. Existing DFT+U PW/LCAO unit tests cover occupation-matrix loading, output helpers, and output frequency gates; existing PW Autotest cases cover DFT+U numerical results. The first-step solver call timing is not checked directly after removing the standalone regression script to keep this PR focused and follow the project's Autotest convention.
  • Core module impact: ESolver_KS prepares ion-step output for both basis paths; PW iteration finish uses existing matrix readiness for both outputs.
  • The PARAM.globalv.global_out_dir reference moved from the PW solver call site to the shared KS solver; the PR-wide global dependency count is unchanged.
  • No governance exception requested.

@mohanchen

Copy link
Copy Markdown
Collaborator

reopen when it is not a draft

@mohanchen mohanchen closed this Nov 1, 2026
@lanshuyue lanshuyue reopened this Nov 1, 2026
@lanshuyue
lanshuyue marked this pull request as ready for review November 1, 2026 17:26
@lanshuyue lanshuyue changed the title Move DFT+U ion-step output preparation to shared KS iteration Unify DFT+U occupation-matrix output across PW and LCAO Nov 1, 2026
@lanshuyue lanshuyue added DFT+U Issues related to DFT plus U function Refactor Refactor ABACUS codes labels Nov 1, 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.

Static review at f34f8ee found no implementation defect in the shared output preparation, readiness gate, or source renames. A regression coverage suggestion follows. No builds, runtime tests or CI diagnostics were performed; author-reported checks were not independently rerun.

Comment thread source/source_esolver/esolver_ks_pw.cpp Resolved
@lanshuyue
lanshuyue force-pushed the codex/unify-dftu-ion-step-output branch from f34f8ee to 7a5aa74 Compare November 2, 2026 05:15

@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 incremental review found that the new DFT+U regression is registered for a build configuration that cannot run it. The original first-step coverage request is addressed by the new assertions; a bounded follow-up is in that thread. No builds or runtime tests were run.

Comment thread tests/01_PW/CMakeLists.txt Resolved
@lanshuyue
lanshuyue force-pushed the codex/unify-dftu-ion-step-output branch from 7a5aa74 to 2bfbea9 Compare November 3, 2026 06:12
@Critsium-xy

Copy link
Copy Markdown
Collaborator

We don't recommend setting up tests this way, i.e. a standalone Python script registered with add_test() that runs ABACUS and checks the output itself. Could you change it into a regular Autotest case (case directory + result.ref)?

lanshuyue commented Nov 3, 2026 •

Copy link
Copy Markdown
Collaborator Author

Thanks for the guidance on Autotest conventions. After reconsidering the scope and existing coverage, I don't think this PR needs an additional regression case or unit test. The existing DFT+U unit tests cover matrix loading and output helpers, and the PW Autotest cases cover numerical results; adding a unit test for those helpers would repeat existing checks. I therefore removed the standalone Python test and its CTest registration in 17071f708 instead of converting it into another Autotest case. The retained tests do not directly check the PW solver's first-step file-write timing, and I've noted that limit in the PR description. The existing PW/LCAO DFT+U unit-test targets passed locally (2/2).

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

DFT+U Issues related to DFT plus U function Refactor Refactor ABACUS codes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants