Repository navigation
Feature: rework out_app_flag as a three-mode output controller with unified output directories, append semantics, and provenance headers - #8159
Conversation
…unified output directories
- Change out_app_flag from bool to int with three states:
0: no append, no subdirectory (overwrite)
1: append to same file (default, backward compatible)
2: write to unified subdirectories
- Add new global directory variables in System_para:
global_psi_dir, global_chg_dir, global_pot_dir,
global_elf_dir, global_dos_dir, global_dip_dir, global_mat_dir
- Initialize directories in set_global_dir():
out_app_flag==2 -> OUT.suffix/{PSI,CHG,POT,ELF,DOS,DIP,MAT}/
otherwise -> all point to global_out_dir (backward compatible)
- Map legacy WFC->PSI, matrix->MAT
- Update all out_app_flag usages from bool to int semantics
- Broadcast new directory variables via Parallel_Common::bcast_string
Issue: deepmodeling#8151
- ctrl_output_fp.cpp: route chg/tau to global_chg_dir, pot/potes to global_pot_dir, ELF to global_elf_dir, dipole to global_dip_dir - ctrl_output_pw.cpp: route wfc to global_psi_dir, dos to global_dos_dir, ldos to global_dos_dir - ctrl_scf_lcao.cpp: route dos to global_dos_dir - ctrl_runner_lcao.cpp: route ldos to global_dos_dir - read_input.cpp: create subdirectories (PSI/CHG/POT/ELF/DOS/DIP/MAT) when out_app_flag==2 - Update docs: out_app_flag semantics from bool to int (0/1/2) Issue: deepmodeling#8151
When out_app_flag defaults to 1 (not 2), all new unified directories (PSI, CHG, POT, ELF, DOS, DIP, MAT) map to global_out_dir for backward compatibility. Update io_sys_var_test.cpp expectations: - global_matrix_dir: "OUT.test/matrix/" -> "OUT.test/" - Add assertions for all 7 new directory variables
- read_input.cpp: change !out_app_flag to out_app_flag == 0 for matrix/WFC legacy directory decisions - Update three unit tests to use int out_app_flag (0/1) instead of bool (false/true), matching the new function signatures: test_cal_dos.cpp, test_write_dos_pw.cpp, write_wfc_nao_test.cpp - Update two comments to describe 0/1 semantics
Replace repeated PARAM.globalv.global_*_dir accesses with the local const variables already declared at function top, eliminating the net +4 PARAM global dependency introduced by the path routing change.
With out_app_flag changed from bool to int, the legacy WFC/ subdirectory is no longer created when out_app_flag=0 (now maps to global_out_dir). Update the multi-k wfc reference path accordingly.
- read_input.cpp: only create PSI/CHG/POT/ELF/DOS/DIP/MAT subdirectories when the corresponding output flag is enabled (out_app_flag=2) - ctrl_scf_lcao.cpp: route write_dmk and angular momentum calculator output to global_mat_dir instead of global_out_dir - hsr_legacy.cpp: route tr_nao.csr to global_matrix_dir - pos_op_writer.cpp: route rr_nao.txt to global_matrix_dir
mohanchen
added
Input&Output
|
@ABACUS-PR-BOT please help review the PR |
ABACUS-PR-BOT
left a comment
ABACUS-PR-BOT
left a comment
There was a problem hiding this comment.
Static review of 00ece15 with independent verification. Five implementation issues and three non-blocking suggestions are noted inline. Whitespace checks passed; the pinned governance check reported warnings only. No build or runtime tests were run, and CI diagnostics were unavailable.
- Remove md_no_append ternary in hsr_legacy.cpp, pos_op_writer.cpp, hterm_writer.cpp, dhs_sparse_writer.cpp: use global_matrix_dir unconditionally (it already equals global_out_dir when out_app_flag != 2, so no behavior change for legacy modes) - write_dmr: add dmr_dir parameter, route DM(R) to MAT instead of hardcoded global_out_dir; drop parameter.h include - read_input.cpp: add out_dmr/out_mat_l/out_mat_dh/out_mat_ds to MAT subdirectory creation flags Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- global_stru_dir now follows out_app_flag: STRU/ for flag 2,
global_out_dir otherwise (consistent with MAT/PSI/CHG pattern)
- relax_driver: route STRU_NOW/STRU{step}/STRU_FINAL through
global_stru_dir instead of global_out_dir
- run_md: drop _MD suffix, use STRU{step} naming consistent with
relax
- global_file: remove unconditional STRU dir creation from
make_dir_out; remove global_stru_dir parameter
- read_input: create STRU dir on demand (out_stru>0 or MD)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- run_md: use out_freq_ion (with md_restartfreq fallback) as the effective ionic-step output frequency for both STRU and restart - run_md: gate STRU output on out_stru > 0, consistent with relax - read_input: simplify STRU dir creation to out_stru > 0 - read_inp_out: update out_stru description to cover MD - read_input_item_md: mark md_restartfreq as deprecated Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- commit 3a38485 made global_stru_dir follow out_app_flag (default 1 maps it to the output dir), but the MD restart lookup still probed STRU_MD_{istep} there, so restart cases whose fixtures live in OUT.suffix/STRU/ (e.g. tests/04_FF/14_LJ_rescale_v) failed to start - search the current global_stru_dir first, then fall back to the legacy OUT.suffix/STRU/ layout; accept both the new STRU{istep+1} naming (1-based, unified with relax) and the legacy STRU_MD_{istep} - quit with a clear diagnostic when no restart structure file exists - add unit test covering both naming schemes in a user read_file_dir
- add PSI/ candidate in read_wfc_nao so RT-TDDFT restart finds mode-2 snapshots - route H(k) term outputs to MAT directory; add out_hsr_npz to MAT dir creation predicate; narrow DOS dir creation predicate to out_dos/out_ldos - pass boolean append to save_mat in write_hk_common (was int out_app_flag) - update out_app_flag/out_wfc_lcao INPUT metadata and position_matrix docs - add mode-2 directory mapping and PSI layout roundtrip unit tests
Conflict resolutions: - read_input_test.cpp: keep both added tests (OutAppFlagParsing ours, ExplicitNoccSurvivesUnresolvedElectronNumber upstream) - relax_driver.cpp: identical condition; keep ours comment (md STRU output now handled by run_md.cpp, md_restartfreq removed) Post-merge verification: make -j 30 in build_max_para_test, 100% Built target abacus_max_para, 0 errors.
In append mode (out_app_flag=1), chg/tau/pot/potes/ELF cube outputs
now reuse a fixed filename (e.g. chgs1.cube, elftot.cube) so every
step overwrites the previous snapshot instead of accumulating
g{N}-suffixed files. Modes 0 and 2 keep the per-step g{N} suffix.
Add an explicit out_app_flag parameter to ModuleIO::write_dmk. In append mode the geometry-step index is dropped from the filename (istep_file = -1) so every ionic step overwrites the previous snapshot, matching the fixed name that read_dmk already expects; the file body still records the real ionic step.
Drop the g{N} suffix from tr_nao.csr and rr_nao.txt in append mode.
The filenames were changing every ionic step, which defeated the
existing MD append branches in save_sparse and the rr assemble path;
with fixed names those branches now accumulate all steps as designed.
Replace the hardcoded out_app_flag=0 in write_wfc_pw (a 2025-05-17
TODO) with an explicit parameter wired from inp.out_app_flag. In the
default append mode the PW wavefunction files now use fixed names
(wfk1_pw.dat), which finally matches what the restart reader
read_wf2rho_pw looks for; modes 0 and 2 keep the g{N} suffix.
write_chg_init/write_pot_init now keep the fixed _ini.cube names in
append mode so each snapshot overwrites the previous one, instead of
accumulating chgs1g{N}_ini.cube / potg{N}_ini.cube files. These
initial-density cubes are write-only snapshots; no reader consumes
them, so no restart path is affected.
Clarify that mode 1 reuses fixed filenames across steps (text records append, cube snapshots overwrite), fix the remaining Boolean wording in out_hsk, and note that out_chg=2/out_pot=3 initial-density cubes omit the geometry-step index in append mode.
The fixed filename alone still truncated the file every ionic step. Add a calculation parameter and open the stream with ios::app under the same condition as open_sparse_file (calculation=="md" && out_app_flag==1 && istep>=0), so each MD step appends a new record while other calculations start fresh.
Rule 22: user-facing output files print ionic steps starting from 1,
matching the g{N} filename suffix convention; zero-based counters such
as "STEP: 0" are not allowed.
Naming (mode 1 fixed name, modes 0/2 keep g{N}):
- DOS (LCAO+PW) and PDOS honor out_app_flag for the g{N} suffix
- lx/ly/lz matrices: Angmom_op::calculate takes out_app_flag
Append (relax now appends like MD; was calculation=="md" only):
- open_sparse_file, rr/dH/dS writers, DM(k): condition relaxed to
(out_app_flag == 1) && (istep >= 0); SCF passes istep=-1, unaffected
- PDOS and lx/ly/lz append per ionic step in mode 1
- DM(k) drops the calculation parameter again (no longer used)
Step indexing (governance rule 22, one-based):
- STEP: headers of sparse text files and the binary CSR step field
now start from 1 (hs_sparse_io, pos_op_writer, dhs_sparse_writer,
hsr_writer)
Build: make -j 30 verified by developer.
Instead of numbered STRU{N} files, out_app_flag=1 now writes one
fixed-name STRU file that accumulates one structure block per written
step, for both relax and MD:
- unitcell::print_stru_file and relax_stru_io::write_stru take a new
append flag; relax writes "# Ionic Step N" headers (one-based,
governance rule 22) via the existing build_stru_header
- mdcell::print_stru_file takes frame_comment + append; MD frames are
prefixed with "# MD step N". The MPI path broadcasts a base offset
(lseek SEEK_END) and keeps absolute pwrite offsets; O_APPEND is
deliberately avoided since Linux pwrite ignores the offset with it
- CIF output keeps single-frame semantics: fixed-name STRU.cif is
overwritten each written step
- modes 0/2 keep the numbered STRU{N} files unchanged
- CLI help of out_stru/out_app_flag documents the mode-1 behavior
Build: make -j 30 verified by developer.
The DM(k) writer emitted the row newline only before each group of elements, so the file ended without a trailing newline. In append mode (out_app_flag == 1) the next " --- Ionic Step N ---" record was glued to the last value of the previous step. Add a trailing newline before closing the file and cover it with a regression assertion.
…ding docs The out_mat_h_* family of INPUT parameters printed k-space matrices in NAO text format, but their docs falsely claimed R-space CSR output (which is gated by WriteHParams::also_hR, always false upstream). Fix the docs to describe the actual k-space output, and rename the parameters to match the established *k suffix convention. - Doc fix: out_mat_h_t/vnl/vl/vh/vxc/exx descriptions now state k-space NAO text output instead of the bogus "R in CSR format". - out_mat_h_t is a legacy alias of out_mat_tk (the dedicated write_h_t path duplicated the T(k) output and is removed). - Rename: out_mat_h_vnl->out_mat_vnlk, out_mat_h_vl->out_mat_vlk, out_mat_h_vh->out_mat_vhk, out_mat_h_vxc->out_mat_vxck, out_mat_h_exx->out_mat_exxk. Old names remain as legacy aliases resolved during input normalization (new name wins; legacy value maps over). - Update MAT/ directory creation condition and ctrl_scf_lcao to the new canonical names. - Add HkComponentAliases serial test covering alias mapping and new-name-wins priority. Verification: compiled with make -j 30; HkComponentAliases, HsOutputAliases and the full MODULE_IO ctest label (54 tests) pass; code_quality_score is unchanged from the pre-change baseline (remaining debt is pre-existing).
mohanchen
changed the title
The out_app_flag=2 structure subdirectory shared the name of the STRU input file, which is confusing in docs, scripts, and on case-insensitive filesystems. Rename it to STRUC/ and update the unit test, INPUT help text, and comments accordingly. MD restart still recognizes structure files written into the legacy STRU/ subdirectory by previous releases.
Under grid-parallel MPI distribution a rank may own no real-space grid points (rho_basis->nrxx == 0). write_h_vh then builds an empty matrix v_h(nspin, 0) and unconditionally dereferences &v_h(ispin, 0), tripping the matrix.h `ic < nc` assertion and aborting the run when out_mat_h_vh / out_mat_h 2 / out_mat_vhks_k is enabled. The SCF path itself is unaffected because it never indexes the matrix this way. Keep setup_veff_hcontainer so the HContainer sparsity stays consistent for the subsequent folding_HR/save_mat, and only skip the gint potential integral when there is no local potential to integrate.
Same class of bug as the preceding Hartree fix: when a grid-parallel rank owns no real-space grid points (nrxx == 0), write_h_vl receives a null v_local from pot->get_fixed_v() and write_h_vxc gets an empty v_xc matrix from XC_Functional::v_xc; both then index the potential unconditionally. Keep setup_veff_hcontainer for a consistent HContainer sparsity and only skip the gint integral when there is no local potential on this rank.
ABACUS-PR-BOT
left a comment
ABACUS-PR-BOT
left a comment
There was a problem hiding this comment.
Static incremental review of 964455d with independent verification. Source inspection confirms the earlier Boolean aliases, indexed root/PSI/WFC lookup, matrix routing, and unused-directory fixes (PR8159-F1, PR8159-F2, PR8159-F4, PR8159-N3). Residual issues and related follow-ups use their existing discussions; the new indexed cumulative-WFC concern is separate from the fixed lookup issue.
New findings and labeled non-blocking suggestions are inline. Whitespace checks passed; the pinned governance checker reported warnings only. No build, runtime, generated-doc or MPI test was run; exact-head CI diagnostics were unavailable.
| // In append mode (out_app_flag == 1) reuse the fixed filename and append | ||
| // each ionic step; single-point SCF passes istep = -1 and truncates. | ||
| const int istep_file = (out_app_flag == 1) ? -1 : istep; | ||
| const bool append_step = (out_app_flag == 1) && (istep >= 0); |
There was a problem hiding this comment.
[P2] Start a fresh accumulation at the first new-run frame
This appends even at the first step of a new run, so rerunning mode 1 in an existing OUT directory retains previous PDOS records. The new DM(k) and STRU accumulators have the same first-step condition; selected L and positive-cadence multi-k relaxation T(R)/r(R) output also gained this behavior. Please separate append-this-frame from mode selection, truncate at a fresh first frame, and preserve append for genuine continuation.
There was a problem hiding this comment.
The fresh-run reset remains partial: MD step 0 now makes append_stru false, which also selects STRU1, so it never truncates the old fixed-name STRU and later frames append to the previous run. Separately, a real MD restart still passes local step 0 through the electronic solver, so the new >0 tests truncate existing PDOS/DM(k) history. Please separate fixed-name selection from append state and propagate an explicit fresh-run/continuation decision through these callers. The earlier bounded L/TR/RR/relax follow-ups in this thread remain unchanged.
There was a problem hiding this comment.
[P2] The fresh MD STRU naming case above is corrected, but the new DMR guard repeats the same name/append coupling at ctrl_scf_lcao.cpp:186. With mode 1 and CSR DMR output, step zero passes append_dmr=false and writes dmrs1g1_nao.csr; later steps use dmrs1_nao.csr, leaving its old content untouched on rerun and omitting the new first frame. A one-step result also fails to refresh the fixed path consumed by init_chg=dm. Please keep the mode-1 filename fixed: write_dmr_csr already truncates at istep<=0. A controller-level 0 -> 1 -> 0 check should verify one filename and removal of old frames.
| std::stringstream file; | ||
| file << PARAM.globalv.global_stru_dir << "STRU_MD_" << mdrun->step_ + mdrun->step_rst_; | ||
| mdcell::print_stru_file(mdcell, mdcell.stru_meta(), file.str()); | ||
| if (param_in.inp.out_stru > 0) |
There was a problem hiding this comment.
[P2] Honor the MD structure format selector
Updated out_stru help now supports MD and defines value 2 as CIF output, but this branch handles 1 and 2 identically and always writes STRU content without a .cif filename. Please implement the MD format choice or explicitly reject/document value 2 as unsupported for MD. Keep the restart structure requirement independent of the user-facing format.
| } | ||
| } | ||
| MPI_Bcast(&header_ok, 1, MPI_INT, 0, comm); | ||
| MPI_Bcast(&base_offset, 1, MPI_OFFSET, 0, comm); |
There was a problem hiding this comment.
[P3] Preserve typed communication through the wrapper boundary
Non-blocking governance note: the pinned rule requires internally guarded parallel wrappers, while this diff adds an MPI_Bcast here and six direct rank/barrier calls in tests. Please use domain-aware wrappers where supported or record a narrow reviewed exception for the offset path. Preserve MPI_Offset width and the cell communicator; narrowing the offset to int or using a world-only broadcast would be unsafe.
There was a problem hiding this comment.
Non-blocking: the new PW append test adds three direct MPI_Barrier calls and the NAO fallback test adds two more. Please use the internally guarded domain-aware barrier where supported, preserving the synchronization around file checks. Retain the earlier MPI_Offset-width and cell-communicator caveat for the separate production broadcast.
There was a problem hiding this comment.
Non-blocking: the latest tests add three more direct MPI calls: MPI_Comm_rank at test_mdcell_reader.cpp:397 and MPI_Barrier at test_read_wfc_nao.cpp:854,884. Please use the communication-domain rank and suitable internally guarded synchronization helpers, or record a narrow reviewed exception. The earlier MPI_Offset-width and cell-communicator caveat for the separate production broadcast still applies.
| // container and zero-iteration invariants that keep empty partitions safe. | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| TEST_F(ElfLogicTest, ZeroGridHasLocalGrid) |
There was a problem hiding this comment.
[P3] Exercise the real empty-partition ELF path
Non-blocking: these zero-grid cases test local lambdas, vector construction and zero-iteration loops without calling write_elf. They would still pass if an empty-rank return were reintroduced before FFT/gather. Please add a production-writer or narrow instrumented MPI test that verifies collective participation with an empty partition; this can follow when runtime validation is available.
| echo "Error: can not find file $ref or $cal" | ||
| return 1 | ||
| fi | ||
| python3 $COMPARE_SCRIPT <(sed '/^[[:space:]]*#/d' "$ref") <(sed '/^[[:space:]]*#/d' "$cal") 4 |
There was a problem hiding this comment.
[P2] Preserve the Vxc comparison tolerance
Non-blocking test-validity note: routing CompareOrbXC through this helper changes accuracy from 5 to 4, so CompareFile.py accepts absolute differences up to 1e-4 instead of 1e-5. The numeric writer still uses precision 16 and the renamed reference is unchanged. Please retain accuracy 5 for Vxc via a helper argument, or document and independently justify the intended tolerance change.
| catch (const std::invalid_argument& e) { | ||
| ModuleBase::WARNING("Input", "out_mat_vnlk enable flag must be 0/1, using default 0"); | ||
| } | ||
| if (count >= 2) try { para.input.out_mat_vnlk[1] = std::stoi(item.str_values[1]); } |
There was a problem hiding this comment.
[P2] Forward the selected matrix-component precision
The new component controls accept and document a second precision value, but write_hk_common always passes literal 8 to save_mat. This affects out_mat_vnlk, out_mat_vlk, out_mat_vhk, out_mat_vxck and out_mat_exxk, including legacy aliases; out_mat_tk already uses precision correctly. Please pass the selected component precision through the writer and verify a non-default value.
| local wfc_ref_nohdr | ||
| wfc_cal_nohdr=$(mktemp) | ||
| wfc_ref_nohdr=$(mktemp) | ||
| sed '/^[[:space:]]*#/d' "$wfc_cal" >"$wfc_cal_nohdr" |
There was a problem hiding this comment.
[P2] Fail comparison when WFC preprocessing fails
In the documented plain-bash collector and Single_job/run_check paths, a missing gamma-only wf_nao.txt makes sed fail but leaves an empty temporary file, and that status is ignored. CompareFile.py loops only over the shorter input, so it records Compare_wfc_lcao_pass 0 against a nonempty reference. Please check source/preprocessing success before comparing; Autotest.sh is protected by bash -e, but these supported entry points are not.
The preceding two commits wrapped the cal_gint_vl call in a pointer guard, so a grid-parallel rank with nrxx == 0 skipped it entirely. cal_gint_vl reaches an MPI_Alltoall in transferSerials2Parallels (via hr_gint_to_hR); with rank 3 absent, ranks 0-2 blocked in the Alltoall while rank 3 raced ahead into save_mat's Allreduce, deadlocking the run. SCF never hangs here because every rank calls cal_gint_vl. Re-enter cal_gint_vl on all ranks to keep the collective matched, and only avoid the original matrix::operator() ic < nc assertion by passing nullptr (never dereferenced, since such a rank owns no local biggrid) instead of &v(ispin, 0) for the Hartree and XC terms. The local term passes v_local directly (already a plain pointer, null on empty ranks) and needed no change, so it is restored to the unconditional call. Verified: examples/17_relax/04_relax_with_output_lcao with out_mat_h_vh=1 under mpirun -np 4 now completes 5 relax steps in ~2s (no assertion, no deadlock; previously it hung).
filename_output only omitted the extra 'k' k-point prefix for hk, sk and tk. The potential terms written by hterm_writer use k-suffixed property names (vhk, vlk, vnlk, vxck, vexxk) that already denote k-space quantities, so they fell into the branch that prepends another 'k', yielding doubled names like vhkk1s1_nao.txt. The INPUT help for out_mat_h_vexx documents the intended pattern as vexxk*_nao.txt (property directly followed by the k-point index), so generalize the special case: any property whose name ends in 'k' takes the bare index, everything else keeps the 'k' prefix. wf, vxc and other non-k properties are unchanged. Verified: examples/17_relax/04_relax_with_output_lcao with out_mat_h_t/vnl/vl/vh/vxc enabled under mpirun -np 4 now writes vhk1s1_nao.txt etc. (no doubled k), and ctest -R MODULE_BASE_filename passes.
find_md_restart_stru only recognized the per-step "STRU{istep+1}" and
legacy "STRU_MD_{istep}" names, so a default MD restart could not find a
checkpoint written in out_app_flag == 1 mode, where run_md appends one
"# MD step N" frame per output step to a single fixed-name STRU file.
Add stru_frame_io::extract_last_frame() in source_cell to split the last
frame of a cumulative STRU file into a single-frame side file, and let
find_md_restart_stru fall back to that file so the restart readers see a
plain single-frame structure. The side file STRU_restart_frame is
rewritten on every restart to stay current with the latest frame.
Cover the frame extraction with a dedicated source_cell unit test
(MODULE_CELL_stru_frame_io) and extend the MD-restart routing case in
MODULE_IO_system_variable_test for the cumulative-STRU layout. Wire the
new source through the cell OBJECT library and Makefile.Objects.
Not yet compiled or run; build and test verification pending.
The mode-1 fixed-name cumulative file was appended to the read candidates unconditionally. Since that file is always read from its first frame, a positive restart index (istep >= 0) would silently load step-0 wave functions whenever the step-suffixed file was missing, corrupting the TDDFT restart without any error. Restrict the cumulative-file candidate to istep < 0 (first read) so a positive restart that cannot find its step-suffixed snapshot now fails loudly through the existing error path instead of reading the wrong frame. Add a unit test (ReadWfcNaoTest.PositiveStepRejectsCumulativeFile) that writes only the cumulative file and asserts that a positive-step read fails. Not yet compiled or run; build and test verification pending.
The new stru_frame_io unit test failed to link: the base library pulls in base_device::memory::* symbols that live in the device target. Add device to the test's link libraries, matching the sibling cell_geometry test.
P1-a made read_set_globalv.cpp call stru_frame_io::extract_last_frame, which lives in the cell library. Test targets that compile read_set_globalv.cpp via the io_input / io_input_serial OBJECT libraries but do not link cell then fail to link with an undefined reference. Add stru_frame_io.cpp to io_input_serial (covers all its consumers) and to the three io_input-based tests that do not link cell. The main io_input library is left untouched: the abacus binary already links cell, so adding the source there would duplicate the symbol.
In append mode (out_app_flag == 1) the cumulative PDOS, DM(k) and MD-STRU files used "istep >= 0" / unconditional append, so rerunning in an existing OUT directory kept stale records from the previous run mixed with the new data. Now the first output step of a fresh (non-restart) run truncates the fixed-name file; later steps and restarted runs keep appending. - write_pdos_text.cpp / write_dmk.cpp: append only when istep > 0 - run_md.cpp: do not append on the very first step of a fresh run (step_ == 0 && step_rst_ == 0) - read_inp_out.cpp: document the mode-1 first-step-truncation contract in the out_app_flag help text
write_wfc_pw used a single istep both to decide whether the ionic step appears in the file name and whether to append a new frame. When out_freq_ion == 0 the caller passes istep = -1 (fixed filename), which made append_frame always false, so every converged ionic step truncated the file instead of accumulating one frame per step as documented. Add an append_step parameter carrying the real output step: append_frame now depends on append_step while istep remains only the filename sentinel. The caller passes the real ionic step; single-point writes pass -1. Add a WriteWfcAppendMode test that truncates on the first step, appends on the second (file grows), and truncates again on a fresh run (size restored). Verified: single-process and mpirun -np 4 both pass (2/2).
…_norm The mode-2 (out_app_flag==2) subdirectory predicates in create_directory were missing three output classes, so enabling them alone wrote files into directories that were never created: - out_hsr (formats 1 and 2, CSR text) writes to global_mat_dir but only out_hsr_npz was in the MAT/ predicate; add out_hsr[0]==1 || ==2. - out_pchg writes to global_chg_dir but only out_chg was in the CHG/ predicate; add !out_pchg.empty(). - out_wfc_norm / out_wfc_re_im write to global_psi_dir but only out_wfc_lcao / out_wfc_pw were in the PSI/ predicate; add !empty() for both. Verified: io_input_serial compiles (make -j 30). The create_directory path cannot be exercised in the serial unit test because make_dir_out calls MPI_Barrier, so predicate coverage is deferred.
ABACUS-PR-BOT
left a comment
ABACUS-PR-BOT
left a comment
There was a problem hiding this comment.
Static incremental review of 720f830 with independent A/B verification. Existing findings and fixes are updated in their original discussions; the new global-dependency finding is inline. git diff --check passed; no build, runtime or CI verification was performed. The inspected target merge has a CMake conflict, and GPU/fp32 empty-partition behavior remains unverified.
| // second write never grew the file. | ||
| TEST_F(ReadWfcRhoTest, WriteWfcAppendMode) | ||
| { | ||
| const int kpar = GlobalV::KPAR; |
There was a problem hiding this comment.
[P2] Keep the PR global-dependency budget non-increasing
This new test adds 11 GlobalV references, changing the exact full PR budget to 53 added and 46 removed (net +7). The established governance includes tests in that budget. Please reuse explicit fixture/setup values and existing locals so the additional regression test does not increase global coupling, or document a reviewed exception; this is static accounting, not a claimed CI result.
|
@ABACUS-PR-BOT Please solve the coflict |
|
Resolved the conflict in 241386f by merging develop and retaining both serial INPUT test dependencies. All 10 focused CPU tests passed, including four-rank MPI INPUT; the affected LCAO solver and PW reader compiled, and governance/whitespace checks passed. GitHub CI has not been verified because the diagnostics connector is unavailable. |
Critsium-xy
left a comment
Critsium-xy
left a comment
There was a problem hiding this comment.
Static review of 720f830 (no build or run). Points already covered by existing bot threads are not repeated. This review focuses on default-behavior regressions and on simplifying the design rather than patching it further.
Main concerns
- Default behavior changed. The code default is
out_app_flag=1(the PR body says 0), and mode 1 now covers much more than the oldtruedid (MD sparse matrices and wfc only): cube, DOS/PDOS, DM(k), T(k), L, STRU, PW wfc, and relax as well as MD. Default runs therefore change file names. For example, LCAO SCFdoss1g1.txtbecomesdoss1.txt, whiletests/performance/catch_properties.shstill reads the old name. Relax/MDchg*.cubeandwf*_pw.datalso become multi-frame, so a laterinit_chg file/init_wfc filereads the first (stale) frame. - Mode 0 is not the legacy layout.
WFC/,matrix/and the MDSTRU/directory are gone, becauseglobal_wfc_dir,global_matrix_dirandglobal_stru_dirall point toOUT/. - INPUT compatibility: see inline comments (
out_eband_terms,md_restartfreq, MD restart without_stru 0).
Suggested direction (this would make most of the open follow-ups unnecessary)
- Restrict mode 1 to what
truecovered before, or make 0 the default and document the renames. - Decouple the MD restart snapshot from the user-facing STRU output, then drop
stru_frame_io, the multi-scheme lookup, and the legacy-directorystat. - Compute the filename and the truncate/append decision once in the ctrl layer and pass them to the writers. The 20+ ad-hoc
(out_app_flag == 1) && istep ...variants already disagree with each other. - Create mode-2 subdirectories at write time (or create all eight up front) instead of maintaining a flag predicate.
- Move unrelated work into separate PRs:
out_mat_*krenames and aliases, the eband/XC refactor, provenance headers, gint empty-grid guards, ELF/dipole MPI fixes, and the Mulliken docs.
Minor: use one provenance-header/timestamp helper instead of three (write_bands.cpp, write_wfc_nao.cpp, vxc_op_tools.cpp) and one spin-tag helper. Comments make up about 26% of the added source lines, well above the surrounding code.
| bool out_dm_npz = false; ///< output DM(R) matrix in npz format | ||
| bool out_app_flag = true; ///< whether output r(R), H(R), S(R), T(R), and dH(R) matrices | ||
| ///< in an append manner during MD liuyu 2023-03-20 | ||
| int out_app_flag = 1; ///< 0: no append, no subdirectory; 1: append to same file; |
There was a problem hiding this comment.
The default stays 1, but mode 1 now drops g{N} and appends for cube, DOS, PDOS, DM(k), T(k), L, STRU and PW wfc, and applies to relax as well as MD. Previously true only affected MD sparse matrices and wfc. Please either keep mode 1 to that scope, or default to 0 and document the renames. The PR body currently says the default is 0.
| // decision uses append_step (the real output step), not istep | ||
| // (the filename sentinel), so an out_freq_ion == 0 run that | ||
| // writes a fixed-name file still appends after its first step. | ||
| const bool append_frame = (append_step > 0 && out_app_flag == 1); |
There was a problem hiding this comment.
With out_freq_ion > 0, filename_output drops both g and e in mode 1, so every written iteration within a step targets the same file. At istep 0 each iteration truncates the file and only the last one survives; at istep > 0 each iteration appends another frame. The docs say e* is kept. Separately, accumulating frames by default means a later init_wfc file reads the first frame, not the latest one.
| if (GlobalV::MY_RANK == 0) | ||
| { | ||
| if(out_app_flag==true) | ||
| if(out_app_flag == 1) |
There was a problem hiding this comment.
Mode 1 always opens the file with ios::app, including at istep 0, so a rerun in the same OUT directory keeps the old DOS blocks. PDOS (write_pdos_text) truncates at istep 0, and the out_dos help also says the first step truncates.
| } | ||
| { | ||
| Input_Item item("out_eband_terms"); | ||
| Input_Item item("out_eband"); |
There was a problem hiding this comment.
This is renamed without an alias, so an existing INPUT that uses out_eband_terms now aborts on an unknown keyword. The out_mat_h_* renames did get aliases. Please keep this consistent, or leave the rename out of this PR.
| /// Effective ionic-step output frequency. | ||
| /// Prefer out_freq_ion; fall back to md_restartfreq for backward | ||
| /// compatibility when out_freq_ion is not set. | ||
| const int eff_output_freq = (param_in.inp.out_freq_ion > 0) |
There was a problem hiding this comment.
When out_freq_ion > 0, md_restartfreq is silently ignored and restart files follow the output cadence instead. Restart cadence and output cadence should stay independent.
| param.input.deepks_out_freq_elec > 0); // xiaohui add 2013-09-01 | ||
|
|
||
| // Create unified subdirectories when out_app_flag == 2, only for outputs the user actually requested | ||
| if (param.input.out_app_flag == 2) |
There was a problem hiding this comment.
This predicate has needed fixes in three review rounds and has to track every writer by hand. Creating the directory at write time, or creating all eight mode-2 directories up front, removes this whole class of bug.
| // truncates the fixed-name cube files and later steps append one complete | ||
| // frame each. Single-point SCF/NSCF runs stop at istep == 0, so they keep | ||
| // overwriting instead of stacking on a stale file from an earlier run. | ||
| const bool append_cube = (out_app_flag == 1) && (istep > 0); |
There was a problem hiding this comment.
This is one of 20+ local variants of the truncate/append decision (istep > 0, istep >= 0, always-append), and they disagree across writers. Computing the filename policy and the append state once and passing them down would fix the inconsistencies, including the restart-at-local-step-0 truncation. Note also that the out_chg/out_pot help still says mode 1 overwrites the file each step.
|
|
||
| const int step = std::max(istep, 0); | ||
| // ionic step written one-based per governance rule 22 | ||
| const int step = std::max(istep, 0) + 1; |
There was a problem hiding this comment.
This changes the meaning of a field in an unversioned binary format, so external readers will silently misread the step. Please either keep the binary records zero-based, or at least add an explicit format/migration note.
| /// Ionic step index of the most recent ionic step, recorded in before_scf(). | ||
| /// Read by after_all_runners() when labelling provenance headers of files | ||
| /// written once at the end of a run. | ||
| int istep_ = 0; |
There was a problem hiding this comment.
This adds mutable workflow state that is written from two places (before_scf and the TDDFT runner), which goes against governance rule 2. Please pass the step explicitly to the output path instead.
| * @param spin_tag label of the collinear spin channel, empty when nspin != 2 | ||
| * @return three comment lines: ABACUS version, local timestamp, ionic step | ||
| */ | ||
| std::string build_band_header(const int istep, const std::string& spin_tag) |
There was a problem hiding this comment.
This is a third copy of the timestamp/header builder (the others are in write_wfc_nao.cpp and vxc_op_tools.cpp), and band_spin_tag duplicates wfc_spin_tag. Please share one helper.
Cache GlobalV::NPROC_IN_POOL/RANK_IN_POOL once in WriteWfcAppendMode and reuse the already-cached kpar/my_rank, instead of re-reading GlobalV at each use site. Reduces the PR's net GlobalV/GlobalC/PARAM reference budget.
Pass global_out_dir/global_matrix_dir/calculation/out_app_flag/nspin/ gamma_only_local/npol/nlocal/cal_force into ModuleIO::output_mat_sparse as explicit arguments instead of reading PARAM inside the function. The sole caller (ctrl_scf_lcao.cpp) already caches most of these values; add the few missing caches there and reuse them (also for h_params.global_matrix_dir). Removes all GlobalV/GlobalC/PARAM reads from output_mat_sparse.cpp and lowers the PR's net global-dependency budget below zero.
io_input (via read_set_globalv.cpp) references stru_frame_io::extract_last_frame. MODULE_IO_input_error_mpi links io_input but not the cell library, causing an undefined reference at link time. Provide the translation unit directly, matching the pattern already used by MODULE_IO_input_test_para, MODULE_IO_parse_args, and MODULE_IO_input_help_test. Verification: code change only; build not rerun locally.
Resolve conflicts with upstream/develop (PR deepmodeling#8225, deepmodeling#8214, deepmodeling#8230, deepmodeling#8223): - write_dipole.cpp grid validation: take upstream version, which rejects only the inconsistent state (nplane == 0 && nrxx != 0) while allowing legitimate empty partitions; a superset of our nplane fix (13b21fb). - write_dipole.cpp file write: keep our MY_RANK == 0 guard (fixes UB of writing to an unopened stream on non-root ranks) wrapping upstream's new setw/scientific output format. - source_io/unittests/CMakeLists.txt: adopt upstream's test/ -> unittests/ rename and test_<module>.cpp file naming (rule 11), while keeping our stru_frame_io.cpp link additions; read_set_globalv.cpp still calls stru_frame_io::extract_last_frame, so the dependency remains. Verification: conflict markers cleared, no stale references to old test file names, all SOURCES entries point to existing files; build not rerun locally.
The previous fix (de91422) restricted the append-mode fixed-name WFC candidate to istep < 0 to prevent positive indexed restarts from silently loading step-0 data. However, that also broke ordinary TDDFT initialization: non-restart TDDFT sets restart_step_ = 0, so istep == 0 could no longer read the fixed-name SCF wavefunction written by out_app_flag 1. Split the candidate selection into three cases: - istep > 0: step-suffixed files only (reject fixed-name fallback) - istep == 0: step-suffixed (g1) first, then fixed-name fallback - istep < 0: fixed-name only (unchanged) Add ReadWfcNaoTest.ZeroStepFallsBackToCumulativeFile covering the istep==0 fallback path. All 14 read_wfc_nao tests pass.
On a restarted MD run, the first local step (step_ == 0) was passed to output writers, causing append-mode writers to truncate the previous run's accumulated records instead of appending to them. Pass the global step index (step_ + step_rst_) to force_virial in both md_base.cpp and run_md.cpp so restarted runs emit a non-zero step and append correctly. Also restructure the STRU output decision in run_md.cpp: separate the fixed-name selection from the append/truncate decision. A fresh run's first frame truncates the fixed-name STRU; later steps and all restarted frames append. Add STRUFreshRunTruncatesPreviousFrames test and wire stru_frame_io.cpp into the neighlist test target.
Several mode-1 accumulators opened their fixed-name output files in append mode even at the first ionic step (istep == 0), so a rerun in an existing OUT directory retained records from the previous run instead of starting fresh. Align all mode-1 accumulators on the same rule: append only when istep > 0; istep == 0 (fresh first frame) and istep == -1 (single-point SCF) truncate. Fixed files: - cal_dos.cpp: add istep > 0 guard to DOS append - angmom_op_mat.cpp: L matrix append was istep >= 0 -> istep > 0 - pos_op_writer.cpp: r(R) append was step >= 0 -> step > 0 - hs_sparse_io.cpp: T(R)/S(R) CSR append was istep >= 0 -> istep > 0 - ctrl_scf_lcao.cpp: DM(R) append had no istep guard -> add istep > 0 Accumulators already correct (no change): PDOS, DM(k), dH/dS, H(k)/S(k) via save_mat, H(R)/S(R) binary via write_hcontainer_csr_binary.
ABACUS-PR-BOT
left a comment
ABACUS-PR-BOT
left a comment
There was a problem hiding this comment.
Restarted LCAO initialization and two mode-1 file-selection paths still need fixes; the details and test notes are inline or in their existing threads. The current source restores ordinary NAO step-zero fallback while retaining positive-step protection, and corrects DOS first-step truncation and the normal fresh MD fixed-name STRU path. Earlier unrelated findings and the deferred checkpoint/frame-alignment concern retain their existing scope.
This is a static revalidation of 4a83ef6 against API base 7ceda1d, including the merge resolutions and later contributor changes; develop was separately recorded at 5a7e6a6. Both diff --check ranges passed. No current-head build, runtime/MPI test, numerical comparison or CI verification was performed; prior conflict-repair test results apply only to 241386f.
| // Pass the global (1 + restarted) step index so that output writers see a | ||
| // non-zero step on a restarted run's first step and append to the | ||
| // accumulated files instead of truncating the previous run's records. | ||
| const int global_step = step_ + step_rst_; |
There was a problem hiding this comment.
[P1] Keep the solver startup index separate from the output index
On an MD restart with a positive saved step, this gives a newly constructed solver a positive index on its first call, even with all outputs disabled. LCAO before_scf then skips the istep==0 DM/HR file-loading branch and rebuilds DMR from newly allocated DMK; the initial wavefunction-to-density path is also gated on zero. Please preserve solver-local initialization at this call and the matching run_md call while passing the absolute output index separately. A restarted LCAO DM/HR case with outputs disabled should cover this boundary.
|
|
||
| if (rank == 0) | ||
| { | ||
| ASSERT_TRUE(stru_frame_io::extract_last_frame(output_file, frame_file)); |
There was a problem hiding this comment.
[P2] Keep the extraction-failure path collective
If extraction fails in this four-rank test, ASSERT_TRUE returns only rank zero from the test. The other ranks continue to print_stru_file below, whose broadcasts/reductions require all ranks, so this failure path can hang instead of reporting the regression. Please broadcast the result and make every rank take the same return decision, or keep the assertion nonfatal until after collective work. This is an unmatched-participation trace, not a reproduced deadlock.
| ModuleBase::world_comm_domain()); | ||
| StruMeta metadata; | ||
| metadata.species.resize(1); | ||
| const std::string output_file = "mdcell_fresh_run_test.STRU"; |
There was a problem hiding this comment.
Non-blocking: these two fixed scratch filenames are created and removed in the working directory. Please choose unique paths under testing::TempDir(), as required by rule 17, and share the selected names across ranks so they operate on the same files. The direct std::remove cleanup is permitted.
| ModuleBase::Matrix3 lattice = make_lattice(); | ||
| MDCell mdcell; | ||
| mdcell.initialize_from_owned_atoms(lattice, | ||
| lattice.Inverse(), |
There was a problem hiding this comment.
Non-blocking: please assign the inverse lattice, communication domain, species labels, masses and counts to named locals before this call. Rule 14 requires computed arguments to be inspectable at the call site; this is a setup/readability issue with no numerical defect asserted.
Summary
This PR reworks
out_app_flagfrom a boolean into a three-mode integer switchand, on top of that, reorganizes ionic-step outputs into unified directories,
fixes several MPI/MD output bugs, and adds machine-readable provenance headers
to user-facing output files.
out_app_flagsemantics (bool -> int)out_app_flagis now anintwith three modes:0(default): legacy per-step file names and layout (unchanged behavior).1: fixed file names; per-ionic-step records are appended to a singleaccumulated file (cube frames, PW/LCAO wavefunctions, band output, DOS/PDOS,
DM(k), T(R)/R(R)/dH/dS sparse matrices, STRU, ...), each record carrying a
one-based
STEP: Nprovenance header.2: per-step files routed into unified subdirectories (STRU/forstructures,
MAT/for sparse matrices such as dmr/tr/rr/dh/ds/lx, etc.),created on demand.
Output renames and provenance headers
Orbitaltonao.txt.kinetic_out->eband_kin,vhartree_out->eband_hart,vpp_local_out->eband_vl,vpp_nonlocal_out->eband_vnl,vxc->eband_vxc), with per-ionic-stepprovenance headers added to band output.
accordingly (manifests updated).
comparison tool skips comment headers.
STEP: 1,matching the
g{N}filename suffix convention), including binary H(R)/S(R)and sparse DMR writers; governance doc updated accordingly.
MD / relaxation structure output
md_restartfreqreplaced byout_freq_ion+out_stru.MD restart STRU lookup broken by the rework is restored.
Bug fixes
write_dipolewhenout_dipole=1.write_elf(fixes ELF output hangs/crashes).out_app_flagand drop the incorrectLCAO-only availability claim.
out_app_flagvalue for PW wavefunction output; use localcached directory variables in
ctrl_output_fp.Tests
out_app_flagnaming and appendsemantics,
save_matoverwrite, andwrite_hskmode-2 rerun semantics.STEPheaders; DM(k) test crashes fixed.(
props_cube.sh,props_mat.sh, LIBRPA manifests, etc.).Backward compatibility
out_app_flag=0) is unchanged; mode 1/2 are opt-in.md_restartfreqis removed in favor ofout_freq_ion+out_stru;existing inputs using
md_restartfreqmust migrate.LIBRPA producer consumers should use the updated manifests.
Verification
source/source_io/test,source/source_lr, and module test directories (three-mode naming/append,sparse writers, STEP headers).
tests/01_PW,02_NAO_Gamma,03_NAO_multik,08_RI, plus validation tools (props_*.sh,check_librpa_producer.py,compare_hsr_binary.py).