Skip to content

Refactor DFT+U occupation reader for strict compact input - #8166

Open
lanshuyue wants to merge 14 commits into
deepmodeling:developfrom
lanshuyue:codex/dftu-base-io-refactor-v2
Open

lanshuyue wants to merge 14 commits into
deepmodeling:developfrom
lanshuyue:codex/dftu-base-io-refactor-v2

Conversation

@lanshuyue

@lanshuyue lanshuyue commented Oct 17, 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

No linked issue. This is a focused follow-up to closed PR #8155, addressing the maintainer request to reduce the size of dftu_base_io.cpp and resubmit the reader refactor when ready.

Unit Tests and/or Case Tests for my changes

  • Commands run:
    • cmake --build build --target MODULE_PW_dftu_base_test -j4
    • cmake --build build --target module_pwdft MODULE_DFTU_base_io MODULE_DFTU_op -j4
    • cmake --build build --target io_input MODULE_IO_input_test_para -j4
    • OMP_NUM_THREADS=1 ctest --test-dir build --output-on-failure -R '^(MODULE_PW_dftu_base_test|MODULE_DFTU_base_io|MODULE_DFTU_op|MODULE_IO_input_test_para)$'
    • make -f source/Makefile BUILD_DIR=/tmp/abacus_make_verify CXX=mpiicpx ... OPENMP=ON -j2
    • git diff --check upstream/develop...HEAD
    • python3 tools/03_code_analysis/agent_governance_check.py --base upstream/develop --head HEAD --format text
    • python3 tools/03_code_analysis/code_quality_score.py --format json source/source_pw/module_pwdft/dftu_base_io.cpp source/source_pw/module_pwdft/dftu_base_io_read.cpp
  • Result summary:
    • All requested CMake build targets passed.
    • The focused CTest targets passed.
    • The legacy Makefile compiled dftu_base_io_read.cpp and linked ABACUS.mpi successfully.
    • git diff --check passed.
    • Before this PR, dftu_base_io.cpp scored 0 (raw score -52) and was 1345 lines, while read_occup_m had cyclomatic complexity 40. After extraction, dftu_base_io.cpp is 996 lines and scores 6; dftu_base_io_read.cpp is below 500 lines and scores 87. The IO/reader average is 46.5. The remaining reader deductions include complexity findings for read_occup_atom_header and read_occup_block_label.
    • Governance check has no blocking findings. Its global-reference warnings are migration-neutral: 2 references moved to the new reader file and 2 were removed from the old file (net 0). The documentation warning is addressed by updating the init_occ_mat INPUT help metadata; generated documentation is not edited directly.
  • Checks not run, with reason:
    • Full ABACUS integration suite was not run. The change is limited to DFT+U occupation-matrix restart parsing and has focused PW, LCAO, parser-error, and INPUT-metadata unit coverage.

What's changed?

  • Extract read_occup_m and its parser helpers into dftu_base_io_read.cpp, keeping the public DFT+U IO declarations together in dftu_base_io.h.
  • Accept only the current compact occupation-matrix format written by occ_mat.txt, with both full-line and inline # comments supported; legacy Atom= / L= / spin= input is rejected with a filename-and-line diagnostic.
  • Replace the unsafe fixed char word[20] token buffer and ignore(150, '\n') calls with line-based std::string parsing.
  • Require exact compact headers, atom order, DFT+U channels, spin/SOC labels, finite values, matrix dimensions, complete atom coverage, and no trailing non-comment content.
  • Parse complete matrix blocks into temporary storage before updating OccupationMatrix.
  • Store parsed matrices through OccupationMatrix::set_flat() instead of accessing ModuleBase::matrix::c directly or using nested element-wise set() loops.
  • Remove the now-redundant init_occ_mat and init_chg reader arguments; file selection remains the caller's responsibility.
  • Remove the legacy dm_onsite.txt fallback and update the init_occ_mat help text to document the compact-only input contract.
  • Add coverage for an O2Fe2 multi-atom cell with an uncorrelated leading species, long and inline comments, long tokens, legacy-header rejection, short/long matrix rows, missing SOC blocks, compact PW SOC, and compact LCAO real spin-basis storage.

Known Limitation and Follow-up

  • This PR does not change the current init_chg = file requirement for a DFT+U occupation-matrix file. If occ_mat.txt is absent, ABACUS still terminates before read_occup_m() is called.
  • Allowing init_chg = file to continue without an occupation-matrix file requires a separate implementation that can initialize or reconstruct the DFT+U occupation matrix safely from the available calculation state.
  • The missing-file error must remain in place until that fallback initialization is implemented and tested. A follow-up change should remove the hard requirement only together with that functionality; it is intentionally out of scope for this compact-reader refactor.

Governance Notes

  • INPUT/docs changes: No INPUT parameter or default changes. The init_occ_mat help metadata now documents the compact-only file format and current filename search order. Generated YAML/Markdown documentation is intentionally not committed.
  • Core module impact: DFT+U restart I/O only. Both PW Pauli storage and LCAO real spin-basis storage remain supported; only legacy text-file compatibility is removed. No ESolver/HSolver public interface changes.
  • Global dependency rationale: GlobalV::MY_RANK and GlobalV::ofs_running were moved with read_occup_m; PR-level added=2, removed=2, net=0.
  • Exceptions requested: none.

@mohanchen
mohanchen self-requested a review October 17, 2026 13:11
@mohanchen mohanchen added DFT+U Issues related to DFT plus U function Refactor Refactor ABACUS codes labels Oct 17, 2026
@mohanchen

Copy link
Copy Markdown
Collaborator

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

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

Reviewed 8e86f9304d1eddf35fc922252913f782bc283f52 statically with an independent reviewer. No new implementation defect was identified; the inline comments are optional test improvements.

Non-blocking: Please refresh the quality-check entry in the PR description. The command names dftu_base_io_read.cpp, but the extracted file is dftu_base_read.cpp; the checked-in scorer reports 87/100 with complexity entries for read_occup_atom_header/read_occup_block_label, rather than 93/100 with none (the IO/reader average is 46.5). Correcting the filename and recorded results is sufficient.

Fresh whitespace and trusted governance checks returned exit 0, with explained migration/documentation warnings. No builds or runtime tests were run; CI diagnostics were unavailable.

}
}

TEST_F(DFTUBaseTest, ReadOccMatCompactHandlesLongComment)

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: Please add a focused multi-atom reader case, including an uncorrelated species, plus missing/duplicate/out-of-order atom variants. The reader now enforces complete ordered coverage, but its unit fixtures all contain one Fe atom, so these paths lack a regression check. A follow-up after merge is reasonable.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. I added a focused positive multi-atom reader case in abef2d9 using a synthetic O2Fe2 cell: the two leading O atoms are uncorrelated and omitted from the file, while Fe atoms 3 and 4 are both read and verified. This directly covers the previously untested species-skip, global-index, and multiple-block paths without changing the shared one-Fe fixture. I kept the existing malformed-header and trailing-content tests for rejection behavior rather than adding three additional death tests. The new case reports 0 ms locally, and all three related DFT+U CTest targets pass.

ASSERT_TRUE(ofs.is_open());
ofs << "Fe Atom 1 L 2 mag 0.0\n";
ofs << "spin 1 nelec 0.5\n";
write_constant_matrix(ofs, 5, 0.1, 6);

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 ends after spin 1, so removing the extra-column check would still make EXPECT_EXIT pass when the reader reaches the missing spin-2 block. Please make the rest of the file valid and change only the row under test; the short-row fixture has the same ambiguity. This can be a follow-up to make these cases catch the row-validation regressions they name.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, fixed in 1b16b3b. Both the short- and long-row fixtures now contain a complete valid spin-2 block, so the row under test is the only malformed part of each file. The focused tests and the related DFT+U test targets pass.

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.

3 participants