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.
| for (std::size_t i = 0; i < mdcell.ghost_atoms().size(); ++i) | ||
| { | ||
| mdcell.ghost_atoms()[i].force = all_force[nowned + i]; |
There was a problem hiding this comment.
Convert ghost forces with the same eV/Angstrom-to-Ry/Bohr factor used for owned atoms below. MD_func immediately returns these values through accumulate_ghost_forces(), which adds them directly to owned forces, so a nonzero periodic-image or cross-rank contribution produces incorrect MD forces. Please cover the ghost-return path with a boundary-crossing regression.
| const double z0 = distance / std::tan(theta0); | ||
| const double r0inv = 1.0 / std::sqrt(rsq + z0 * z0); | ||
| const double ar = r0inv * z0; |
There was a problem hiding this comment.
Handle the finite radial limit at distance == rmin0 in both descriptor paths. A positive rmin0 is accepted, but this expression then forms r/tan(0) followed by 0*infinity, producing non-finite higher-order components; the zero-distance guard does not cover it. Please add value-and-force checks at and near a positive rmin0.
| maximum_cutoff = std::max(maximum_cutoff, 2.0 * parameters.element_radii[i] * parameters.rcutfac); | ||
| } | ||
| parameters.rcut = maximum_cutoff; | ||
| return parameters.valid(error); |
There was a problem hiding this comment.
Validate rmin0 against every active pair cutoff. Radii 0.5 and 2 with rcutfac=1 and rmin0=1.5 pass this maximum-cutoff check, but the small-small cutoff is 1: the value path retains a neighbor at distance 1.2 while the derivative path excludes it. Please reject invalid pair intervals or apply one consistent radial-domain policy.
| parameters.chem_flag = value != 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
Reject unsupported semantic SNAP parameters explicitly. A file containing switchinnerflag 1 plus sinner/dinner is accepted here, but those settings never reach the descriptor, so the requested short-range switching is omitted from its energy and forces. Please diagnose unsupported options, or implement their value and derivative behavior.
| found[element_index] = true; | ||
| parameters.element_radii[element_index] = radius; | ||
| parameters.element_weights[element_index] = weight; | ||
| coefficients[element_index] = values; |
There was a problem hiding this comment.
Non-blocking: Document the chemical-channel order expected in STRU when importing a model, and add a test of that contract. Coefficient rows are matched by name, but channel axes follow STRU order, so changing species order can reinterpret the same flat coefficient vector.
| const ModuleMD::SNAP::ForceResult analytic = calculator.compute_with_analytic_force(neighbors, 0); | ||
| const ModuleMD::SNAP::ForceResult finite = calculator.compute_with_finite_difference_force(neighbors, 0, 1.0e-6); |
There was a problem hiding this comment.
Non-blocking: Add checked-in force comparisons for a non-collinear, multi-neighbor model with twojmax >= 2 and nonzero angular coefficients, plus a quadratic model and a strain check for the virial. The current calculator finite-difference tests use twojmax=0, while the fixed descriptor reference checks values only.
| { | ||
| const UWithDerivative u_derivative = | ||
| descriptor_evaluator_.compute_u_with_derivative(neighbors[neighbor_index], direction); | ||
| std::vector<std::complex<double> > u = descriptor_evaluator_.accumulate_u(neighbors, central_element); |
There was a problem hiding this comment.
Non-blocking: Accumulate u once before these loops and reuse it for each derivative. The environment is unchanged, but accumulate_u traverses all neighbors on each of the 3*N iterations, adding redundant recurrences and allocations to the force path.
| return false; | ||
| } | ||
|
|
||
| std::ifstream coeff_stream(coefficient_file.c_str()); |
There was a problem hiding this comment.
Non-blocking: Record the full coefficient and parameter paths after successful reads in stdout and the running log, as required by the file-I/O rule. Only failures identify the files currently; the existing injected running_log can support successful-read confirmation in the solver wrapper.
| snap_virial_(row, column) += relative[row] * pair_force[column] | ||
| / (ModuleBase::Ry_to_eV * mdcell.omega()); |
There was a problem hiding this comment.
Non-blocking: Move the common cell-level force and virial assembly into module_snap and call it as a named solver step, following the ESolver module rule. The MDCell and UnitCell loops currently duplicate physical assembly and unit conversion outside the calculator tests.
| const Dual x(neighbor.x, direction == 0 ? 1.0 : 0.0); | ||
| const Dual y(neighbor.y, direction == 1 ? 1.0 : 0.0); | ||
| const Dual z(neighbor.z, direction == 2 ? 1.0 : 0.0); |
There was a problem hiding this comment.
Non-blocking: Assign these conditional derivative seeds to named locals before passing them to Dual, following the inspectable-argument rule. Apply the same cleanup to the new numerical-call conversions, and move the computed twojmax member clamp into the constructor body.
Reminder
AGENTS.mdanddocs/developers_guide/agent_governance.md.Linked Issue
This PR does not require a linked issue because it adds a standalone CPU SNAP potential implementation and corresponding tests.
Unit Tests and/or Case Tests for my changes
Commands run