Repository navigation
Conversation
|
@ABACUS-PR-BOT Could you review the PR? |
ABACUS-PR-BOT
left a comment
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Reminder
AGENTS.mdanddocs/developers_guide/agent_governance.md.source/changes.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.cppand resubmit the reader refactor when ready.Unit Tests and/or Case Tests for my changes
cmake --build build --target MODULE_PW_dftu_base_test -j4cmake --build build --target module_pwdft MODULE_DFTU_base_io MODULE_DFTU_op -j4cmake --build build --target io_input MODULE_IO_input_test_para -j4OMP_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 -j2git diff --check upstream/develop...HEADpython3 tools/03_code_analysis/agent_governance_check.py --base upstream/develop --head HEAD --format textpython3 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.cppdftu_base_io_read.cppand linkedABACUS.mpisuccessfully.git diff --checkpassed.dftu_base_io.cppscored 0 (raw score -52) and was 1345 lines, whileread_occup_mhad cyclomatic complexity 40. After extraction,dftu_base_io.cppis 996 lines and scores 6;dftu_base_io_read.cppis below 500 lines and scores 87. The IO/reader average is 46.5. The remaining reader deductions include complexity findings forread_occup_atom_headerandread_occup_block_label.init_occ_matINPUT help metadata; generated documentation is not edited directly.What's changed?
read_occup_mand its parser helpers intodftu_base_io_read.cpp, keeping the public DFT+U IO declarations together indftu_base_io.h.occ_mat.txt, with both full-line and inline#comments supported; legacyAtom=/L=/spin=input is rejected with a filename-and-line diagnostic.char word[20]token buffer andignore(150, '\n')calls with line-basedstd::stringparsing.OccupationMatrix.OccupationMatrix::set_flat()instead of accessingModuleBase::matrix::cdirectly or using nested element-wiseset()loops.init_occ_matandinit_chgreader arguments; file selection remains the caller's responsibility.dm_onsite.txtfallback and update theinit_occ_mathelp text to document the compact-only input contract.Known Limitation and Follow-up
init_chg = filerequirement for a DFT+U occupation-matrix file. Ifocc_mat.txtis absent, ABACUS still terminates beforeread_occup_m()is called.init_chg = fileto 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.Governance Notes
init_occ_mathelp metadata now documents the compact-only file format and current filename search order. Generated YAML/Markdown documentation is intentionally not committed.GlobalV::MY_RANKandGlobalV::ofs_runningwere moved withread_occup_m; PR-level added=2, removed=2, net=0.