Repository navigation
Conversation
…evelop # Conflicts: # source/source_io/module_parameter/read_inp_sys.cpp
ABACUS-PR-BOT
left a comment
There was a problem hiding this comment.
The SNAP MD path mixes ghost and owned force units, and accepted model settings can produce inconsistent or non-finite results. Please correct the force handoff and validate or handle those model settings.
Static source and analytic review at 47543b3 against e5ad530, with separate compatibility inspection of develop 5a7e6a6. No build, tests, numerical/MPI runs or CI diagnostics were executed; the author’s reported checks were not independently verified.
Non-blocking: Correct the PR INPUT notes: snap_param_file is newly added, and snap becomes an accepted esolver_type value. The C++ metadata registrations are already present.
ABACUS-PR-BOT
left a comment
There was a problem hiding this comment.
One radial edge case and three nonblocking items remain; the existing radial, chemical-order and argument-style threads contain the follow-ups. Source inspection confirms the ghost-force conversion, pair-cutoff checks, inner switch, common cell accumulator, environment reuse, logging and angular/strain test additions.
Static review at 8fbec3f; no builds, runtime tests or CI diagnostics were run. Author-reported test results were not independently reproduced.
|
@19hello Only one question: why SNAP but not other CPU-running machine learning potential ? |
Someone needs this potential for research, more empirical/ML potentials will be added in future |
ABACUS-PR-BOT
left a comment
There was a problem hiding this comment.
The zero-weight guard, named numerical products and CG index address the previous requests in source. One MPI diagnostic gap remains below. Static review only; no build or runtime/MPI tests were run.
ABACUS-PR-BOT
left a comment
There was a problem hiding this comment.
Static incremental review at 0a4bda2 covered model validation, rank-visible errors, cached descriptors and analytic-force contractions. I did not run builds, tests or MPI calculations.
…evelop # Conflicts: # source/source_esolver/CMakeLists.txt
Reminder
Linked issue
Standalone CPU SNAP implementation; no linked issue.
What changed
Use
SNAP model parameters and chemical-channel ordering are documented in docs/advanced/md.md.
Verification
Commands run:
The Autotest command ran from /tmp/pr8235-regression/04_FF outside the sandbox with all five checked-in SNAP cases copied there. Build passed; 6/6 focused CTest tests passed; 20/20 numerical case comparisons passed. The six quality-scored changed files passed. The governance checker returned no blocking errors; its review warnings are addressed below. The three-atom case gives 54.909561 eV and Pxx -122.909021 kbar, consistent with the LAMMPS reference in its README; its force components and strain derivative are checked by the unit test. A direct UnitCell single-point run gave the same energy, force, and stress.
Governance notes
Follow-up: model filenames and source naming (2026-11-02)
tests/SIMULATION_MODELSand named each pair by its CPU regression case number:snap_102,snap_103,snap_104,snap_105, andsnap_106(.coeffand.param). Each case INPUT points to its matching pair. Identical fixture contents are retained where two cases intentionally use the same potential, so the case-to-file mapping is explicit.source/source_md/module_snap/unittestand descriptive SNAP source, test, class, function, and variable names. The derivative implementation uses the shorter but descriptivesnap_derivative.cpp. CMake and the legacy Makefile object list were updated; pyabacus has no SNAP-specific source entries.ESolver_SNAP::runnercatches them and callsWARNING_QUIT, so runtime errors give a clear diagnostic on stdout and inwarning.logand the running log./tmp/pr8235-regression/04_FFwithtests/SIMULATION_MODELScopied to the parent directory.rmin0 2.1exited 1 withSNAP evaluation failed: SNAP neighbor distance must be greater than rmin0.The message appeared in stdout,OUT.autotest/warning.log, andrunning_md.log.code_quality_score.py.Follow-up: Clebsch–Gordan filenames (2026-11-02)
Renamed the implementation and header to
snap_cg.cpp/snap_cg.h, the matching test source totest_snap_cg.cpp, and its CTest target toMODULE_MD_SNAP_cg. Updated the descriptor include, CMake source lists, and legacy Makefile object list. This is a naming-only change: full CMake build and all 4 SNAP unit tests passed. The staged governance check had no blocking errors; its header warning reflects the required complete CG table type insnap_descriptor.h, and no user documentation change is needed.Follow-up: review comments and CI rerun (2026-11-02)
ZeroWeightSkipsInvalidRadiusto check isolated energy, zero forces, and nonzero-weight rejection.ChemicalAngularChannelsFollowElementOrderwithtwojmax=2, a mixed chemical triplet, both neighbor forces versus finite differences, and equivalent energy/forces after swapping element order and permuting coefficient axes.cmake --build /tmp/abacus-snap-check2 -j2passed;ctest --test-dir /tmp/abacus-snap-check2 --output-on-failure -R 'MODULE_MD_SNAP_|MODULE_IO_(read_input_serial|read_item_serial)'passed 6/6;OMP_NUM_THREADS=1 bash ../integrate/Autotest.sh -a /tmp/abacus-snap-check2/abacus_pw_ser -n 1 -r '10[23456]_SNAP_'passed 20/20 numerical comparisons from/tmp/pr8235-regression/04_FFoutside the sandbox.git diff origin/develop...HEAD --checkand the PR-level governance checker passed without blocking errors. The new map and tuple header includes own a value member and are required. GitHub Integration Test and Unit Test job was explicitly rerun; the push also requests fresh CI on this head.Follow-up: explicit SNAP channels and MPI errors (2026-11-02)
accumulate_uoverload; callers now state the central element explicitly. Removed unusedh_coefficient,coefficient_count,descriptor_sizeaccessor, and the unconsumed derivative value array. These are internal cleanups; the user-facing SNAP model and INPUT syntax are unchanged.ESolver_SNAPno longer readsGlobalVdirectly. Model paths are printed once each to rank-0 stdout and to the running log; these are two output destinations, not duplicate evaluations.WARNING_QUIT, because the existing MPI setup disables nonzero-rank stdout and default per-rank logs. Added04_FF_SNAP_rank_error, which places only the bad close pair on rank 1 and checks nonzero exit, rank-1 stderr, absence of the error on rank-0 stdout, and one stdout confirmation per model file.abacus_pw_parabuild passed;LD_LIBRARY_PATH=/usr/lib/x86_64-linux-gnu/openblas-pthread:$LD_LIBRARY_PATH ctest --test-dir /tmp/abacus-snap-build-develop-mpi --output-on-failure -R '^04_FF_SNAP_rank_error$'passed 1/1 outside the sandbox. The library path override is needed on this workstation because its systemlibblas.so.3and toolchain OpenBLAS otherwise collide (undefined symbol: gotoblas). Python syntax check, diff whitespace check, and PR-level governance check passed without blocking errors. Governance notes: the factory is the existing top-level boundary where the global running log and rank are injected; the PR-level global dependency delta remains -2. No docs update is needed for the internal API cleanup; the stderr diagnostic improves error visibility.Follow-up: model validation and descriptor reuse (2026-11-02)
WARNING_QUITwith element, expected count, and actual count.74259aa10:cmake -S . -B build_parallel_md1_current -UENABLE_RAPIDJSONpassed;cmake --build build_parallel_md1_current --target MODULE_MD_SNAP_model MODULE_MD_SNAP_calculator -j8,cmake --build build_parallel_md1_current --target MODULE_MD_SNAP_cg MODULE_MD_SNAP_descriptor -j8,cmake --build build_parallel_md1_current --target esolver -j8, andcmake --build build_parallel_md1_current --target abacus_basic_para -j8passed;LD_LIBRARY_PATH=/tmp:$LD_LIBRARY_PATH ctest --test-dir build_parallel_md1_current --output-on-failure -R '^MODULE_MD_SNAP_'passed 4/4. The/tmplibrary path maps this host's broken system BLAS/LAPACK to the existing OpenBLAS library; without it, runtime linking fails withundefined symbol: gotoblas./tmpcopy passed with 54.909561 eV. A malformed 22-coefficient model (21 expected) exited 1 and wrote the mismatch towarning.log.git diff --checkpassed. The staged governance checker returned only a documentation-sync review warning; the user interface and documented behavior did not change.Follow-up: rfac0 range (2026-11-02)
0 < rfac0 <= 1in the SNAP model to keep the radial angle from crossing pi inside the pair cutoff. The zero-distance handling andstd::domain_errorpath were left unchanged after discussion.docs/advanced/md.md; add direct boundary and parameter-file tests.0a4bda26f:cmake --build build_parallel_md1_current --target MODULE_MD_SNAP_model MODULE_MD_SNAP_calculator -j8passed;LD_LIBRARY_PATH=/tmp:$LD_LIBRARY_PATH ctest --test-dir build_parallel_md1_current --output-on-failure -R '^MODULE_MD_SNAP_'passed 4/4;cmake --build build_parallel_md1_current --target abacus_basic_para -j8passed. One-process case 105 from a/tmpcopy exited 0, and the same case withrfac0 1.2exited 1 withSNAP rfac0 must satisfy 0 < rfac0 <= 1.in stdout.git diff --checkpassed;python3 tools/03_code_analysis/agent_governance_check.py --stagedreported no findings. This host requires the temporary/tmpOpenBLAS mapping described above for runtime linking.Follow-up: latest robot review and develop merge (2026-11-03)
origin/developinae972f346. The only conflict wassource/source_esolver/CMakeLists.txt; kept the upstream list format and retainedesolver_snap.cpp. GitHub reports the PR as mergeable.3a554aee0, added a two-element isolated chemicalbzeroregression withwselfallflag=0and nonzero diagonal-channel coefficients; built the coefficient-count diagnostic in a named string; and restored the device summary to both stdout and the running log while collecting hardware information once. Replied to and resolved all three new robot review threads.cmake -S . -B build_parallel_md1_current;cmake --build build_parallel_md1_current --target abacus_basic_para -j8;LD_LIBRARY_PATH=/tmp:$LD_LIBRARY_PATH ctest --test-dir build_parallel_md1_current --output-on-failure -R '^MODULE_MD_SNAP_'(4/4 passed);LD_LIBRARY_PATH=/tmp:$LD_LIBRARY_PATH ctest --test-dir build_parallel_md1_current --output-on-failure -R '^04_FF_SNAP_rank_error$'(1/1 passed outside the sandbox). A one-process SNAP 105 run withOMP_NUM_THREADS=1exited 0 at 54.909561 eV and printed the device summary once to stdout and once torunning_md.log. The sandboxed MPI attempt could not launch because OpenMPI socket creation was denied; the outside-sandbox rerun passed.git diff --cached --checkand the PR-level governance checker passed without blocking errors.