Repository navigation
Conversation
Gather frame text through CommunicationDomain and write rows by the stable type-major index. Emit frame separators once, including when some MPI ranks own no atoms. Add serial-launch and four-rank tests for reversed ownership, an empty rank, empty frames, and append mode. No documentation update needed: this restores trajectory ordering without changing INPUT parameters, units, or columns.
Use two synthetic atoms and Gamma plus a non-Gamma point. Exercise the real MD update and backing-cell sync for sub-roundoff negative motion, and check H/S/coefficient phase covariance for all axes, both crossing directions, both atoms, and clearing a previous shift. Verify the test fails when the coordinate fix is reverted or the Bloch phase sign is reversed. Restore both mutations and pass six related CTest targets, including the new single- and four-rank RT tests. No documentation update required: tests only.
…del" This reverts commit 62cf361.
Add INPUT, STRU and KPT for two H atoms on a periodic face with one non-Gamma k point and a supplied sub-roundoff transverse velocity. Register the case in CASES_CPU.txt and check post-initial SCF iterations and convergence through an opt-in md_scf.ref collector. Original beta11 fails Autotest.sh with both one and four MPI ranks: step 2 reaches scf_nmax=8 without convergence. The fixed binary and beta11 with only the coordinate patch pass with one step-2 iteration. Existing 16_NO_vel_TDDFT and the four focused MD CTest targets also pass. Document the trigger and commands in the case README.
Raise scf_nmax to 100, remove the opt-in SCF collector and its reference file, and keep the case README to its single purpose sentence. With one and four MPI ranks, the fixed case passes in one second-step iteration. Original beta11 takes ten iterations but reaches an energy about 79 eV away from the reference, so the existing Autotest.sh energy comparison fails without a custom validator or timeout.
ESROAMER
added
Bugs
ABACUS-PR-BOT
left a comment
ABACUS-PR-BOT
left a comment
There was a problem hiding this comment.
Static review of d2f1f5fb00c8 completed with independent verification. Two conditional P2 issues and one nonblocking naming note are below. No builds or runtime tests were run; the H2 energy/trigger and reported beta11 comparison remain unverified, and CI diagnostics were unavailable.
| if (total_size + lengths[rank] > maximum_count) | ||
| { | ||
| total_overflow = 1; | ||
| break; |
There was a problem hiding this comment.
[P2] This introduces an aggregate INT_MAX-byte limit on MD_dump, although the previous writer used MPI_Offset and wrote each rank's local text. A supported one-atom cell_replica 256 256 256 system needs at least 2.53 GB with the default position/force/velocity columns, so this throws even when every local payload fits. Please gather/write ordered bounded batches so the ordering fix preserves large distributed dumps; count planning can be tested with a small injected limit. (8207-A1)
There was a problem hiding this comment.
Addressed in 04a1ba3a26f3: ordered index batches now bound each collective independently of frame size. The bound and small-budget/empty-rank test assertions were checked statically at d4028da3410d; no MPI run was performed. (8207-A1)
| COMMAND ${MPIEXEC_EXECUTABLE} ${MPIEXEC_NUMPROC_FLAG} 4 | ||
| $<TARGET_FILE:MODULE_MD_dump_mpi> |
There was a problem hiding this comment.
[P2] Please forward MPIEXEC_PREFLAGS and MPIEXEC_POSTFLAGS here, as the other MPI CTest commands do. When a required launcher option is supplied through these CMake variables, this test drops it and can fail before exercising the dump checks. Empty/default flag configurations are unaffected. (8207-B1)
There was a problem hiding this comment.
Addressed in d1e2235b0191: the four-rank test command now forwards both configured MPI launcher flag variables. Verified in source at d4028da3410d; no launcher run was performed. (8207-B1)
| TARGET MODULE_MD_dump_mpi | ||
| KEEP_FEATURE_DEFINITIONS __MPI | ||
| LIBS parameter psi device | ||
| SOURCES test_md_func_mpi.cpp ../../source_base/parallel_common.cpp |
There was a problem hiding this comment.
Non-blocking: the new-test naming rule calls for test_md_func.cpp to match md_func.cpp. An MPI subdirectory can preserve that name and the separate MPI target, or a narrow naming exception can be recorded for maintainer review. Follow-up after merge is acceptable. (8207-A2)
There was a problem hiding this comment.
Addressed in d1e2235b0191: the MPI source is now mpi/test_md_func.cpp, and CMake points to that path. Statically verified at d4028da3410d. (8207-A2)
| 26_PW_single_TDDFT | ||
| 27_PW_gmres_recycle_TDDFT | ||
| 28_PW_gmres_subspace_TDDFT | ||
| 29_NO_MD_boundary_shift |
There was a problem hiding this comment.
A gentle suggestion: could we reuse the existing NAO RT tests (01-20)? I've noticed that some of these tests are redundant. For example, 10_NO_HHG_TDDFT does nothing more than apply two Gaussian pulses, and such cases involving multiple external fields are already covered by the much stronger 20_NO_MIXED_EFIELD_TDDFT.
So perhaps your new test could replace test No. 10? It seems a bit odd to place an NAO RT test after the PW RT tests...
There was a problem hiding this comment.
A gentle suggestion: could we reuse the existing NAO RT tests (01-20)? I've noticed that some of these tests are redundant. For example,
10_NO_HHG_TDDFTdoes nothing more than apply two Gaussian pulses, and such cases involving multiple external fields are already covered by the much stronger20_NO_MIXED_EFIELD_TDDFT.So perhaps your new test could replace test No. 10? It seems a bit odd to place an NAO RT test after the PW RT tests...
That’s a good suggestion—let me revise it.
There was a problem hiding this comment.
Non-blocking: cases 05/06 retain finite-field length-gauge force/stress checks, and case 20 covers mixed profiles in hybrid gauge. The remaining distinction is the finite two-field scalar-potential sum in H_TDDFT_pw::cal_fixed_v: case 20 bypasses it, and this replacement reaches it only with weak nonzero tails. A small accumulation assertion or a link to an equivalent oracle would cover that combination; follow-up after merge is fine and need not disturb the boundary trigger. (8207-B2)
ABACUS-PR-BOT
left a comment
ABACUS-PR-BOT
left a comment
There was a problem hiding this comment.
Static follow-up of d4028da3410d with independent verification found the three prior issues addressed. No new product defect is confirmed; the remaining notes are nonblocking.
Non-blocking: please update the integration command to ^10_NO_MD_boundary_shift$ and state which revision the reported results cover. The old ^29_NO_MD_boundary_shift$ filter now selects no cases, and Autotest can report a zero-case pass, so the documented command does not exercise this revision. (8207-A3)
No builds or runtime tests were run. The H2 reference, actual boundary trigger, beta11 comparison, MPI/serial/GPU execution and CI remain unverified.
Linked Issue
Fixes #8206.
What's changed?
[0, 1).MD_dump, using bounded collective batches for large frames.29_NO_MD_boundary_shift.Unit Tests and/or Case Tests for my changes
CPU build and the following tests passed:
ctest --test-dir build-fix --output-on-failure \ -R '^MODULE_MD_(func|verlet|dump_mpi(_np4)?)$'From
tests/05_rtTDDFT, using the fixed executable:The integration case passes with both 1 and 4 MPI ranks. Unmodified beta11 fails the existing energy comparison by approximately 79 eV.
MD tests cover one-row and small-byte batches, aggregate overflow with individually fitting rank payloads, empty ranks, and appended frames. The 4-rank CTest also passes with nonempty
MPIEXEC_PREFLAGSandMPIEXEC_POSTFLAGS;16_NO_vel_TDDFTpasses with 4 MPI ranks.No documentation update required: no INPUT parameters or output fields change.
The include is required by the new CommunicationDomain API.