Skip to content

Commit 00f9a40

Browse files
fix(test): address code review comments on virial validation
- Remove accidentally committed log.lammps file - Fix virial validation to assert non-empty instead of silently skipping - Fix atom_vir sizing to use (natoms + 2) * 9 layout consistently - Fix virial variable name/index mismatch in LAMMPS spin test Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
1 parent 1415d11 commit 00f9a40

File tree

4 files changed

+22
-57
lines changed

4 files changed

+22
-57
lines changed

log.lammps

Lines changed: 0 additions & 23 deletions
This file was deleted.

source/api_cc/tests/test_deeppot_dpa_pt_spin.cc

Lines changed: 17 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -149,9 +149,7 @@ TYPED_TEST(TestInferDeepSpinDpaPt, cpu_build_nlist) {
149149
EXPECT_LT(fabs(force[ii] - expected_f[ii]), EPSILON);
150150
EXPECT_LT(fabs(force_mag[ii] - expected_fm[ii]), EPSILON);
151151
}
152-
if (virial.empty()) {
153-
return;
154-
}
152+
EXPECT_FALSE(virial.empty()) << "Virial should not be empty";
155153
EXPECT_EQ(virial.size(), 9);
156154
for (int ii = 0; ii < 3 * 3; ++ii) {
157155
EXPECT_LT(fabs(virial[ii] - expected_tot_v[ii]), EPSILON);
@@ -186,18 +184,15 @@ TYPED_TEST(TestInferDeepSpinDpaPt, cpu_build_nlist_atomic) {
186184
EXPECT_LT(fabs(force[ii] - expected_f[ii]), EPSILON);
187185
EXPECT_LT(fabs(force_mag[ii] - expected_fm[ii]), EPSILON);
188186
}
189-
if (!virial.empty()) {
190-
EXPECT_EQ(virial.size(), 9);
191-
for (int ii = 0; ii < 3 * 3; ++ii) {
192-
EXPECT_LT(fabs(virial[ii] - expected_tot_v[ii]), EPSILON);
193-
}
187+
EXPECT_FALSE(virial.empty()) << "Virial should not be empty";
188+
EXPECT_EQ(virial.size(), 9);
189+
for (int ii = 0; ii < 3 * 3; ++ii) {
190+
EXPECT_LT(fabs(virial[ii] - expected_tot_v[ii]), EPSILON);
194191
}
195192
for (int ii = 0; ii < natoms; ++ii) {
196193
EXPECT_LT(fabs(atom_ener[ii] - expected_e[ii]), EPSILON);
197194
}
198-
if (atom_vir.empty()) {
199-
return;
200-
}
195+
EXPECT_FALSE(atom_vir.empty()) << "Atomic virial should not be empty";
201196
EXPECT_EQ(atom_vir.size(), natoms * 9);
202197
for (int ii = 0; ii < natoms * 9; ++ii) {
203198
EXPECT_LT(fabs(atom_vir[ii] - expected_atom_v[ii]), EPSILON);
@@ -331,9 +326,7 @@ TYPED_TEST(TestInferDeepSpinDpaPtNopbc, cpu_build_nlist) {
331326
EXPECT_LT(fabs(force[ii] - expected_f[ii]), EPSILON);
332327
EXPECT_LT(fabs(force_mag[ii] - expected_fm[ii]), EPSILON);
333328
}
334-
if (virial.empty()) {
335-
return;
336-
}
329+
EXPECT_FALSE(virial.empty()) << "Virial should not be empty";
337330
EXPECT_EQ(virial.size(), 9);
338331
for (int ii = 0; ii < 3 * 3; ++ii) {
339332
EXPECT_LT(fabs(virial[ii] - expected_tot_v[ii]), EPSILON);
@@ -368,18 +361,15 @@ TYPED_TEST(TestInferDeepSpinDpaPtNopbc, cpu_build_nlist_atomic) {
368361
EXPECT_LT(fabs(force[ii] - expected_f[ii]), EPSILON);
369362
EXPECT_LT(fabs(force_mag[ii] - expected_fm[ii]), EPSILON);
370363
}
371-
if (!virial.empty()) {
372-
EXPECT_EQ(virial.size(), 9);
373-
for (int ii = 0; ii < 3 * 3; ++ii) {
374-
EXPECT_LT(fabs(virial[ii] - expected_tot_v[ii]), EPSILON);
375-
}
364+
EXPECT_FALSE(virial.empty()) << "Virial should not be empty";
365+
EXPECT_EQ(virial.size(), 9);
366+
for (int ii = 0; ii < 3 * 3; ++ii) {
367+
EXPECT_LT(fabs(virial[ii] - expected_tot_v[ii]), EPSILON);
376368
}
377369
for (int ii = 0; ii < natoms; ++ii) {
378370
EXPECT_LT(fabs(atom_ener[ii] - expected_e[ii]), EPSILON);
379371
}
380-
if (atom_vir.empty()) {
381-
return;
382-
}
372+
EXPECT_FALSE(atom_vir.empty()) << "Atomic virial should not be empty";
383373
EXPECT_EQ(atom_vir.size(), natoms * 9);
384374
for (int ii = 0; ii < natoms * 9; ++ii) {
385375
EXPECT_LT(fabs(atom_vir[ii] - expected_atom_v[ii]), EPSILON);
@@ -464,18 +454,15 @@ TYPED_TEST(TestInferDeepSpinDpaPtNopbc, cpu_lmp_nlist_atomic) {
464454
EXPECT_LT(fabs(force[ii] - expected_f[ii]), EPSILON);
465455
EXPECT_LT(fabs(force_mag[ii] - expected_fm[ii]), EPSILON);
466456
}
467-
if (!virial.empty()) {
468-
EXPECT_EQ(virial.size(), 9);
469-
for (int ii = 0; ii < 3 * 3; ++ii) {
470-
EXPECT_LT(fabs(virial[ii] - expected_tot_v[ii]), EPSILON);
471-
}
457+
EXPECT_FALSE(virial.empty()) << "Virial should not be empty";
458+
EXPECT_EQ(virial.size(), 9);
459+
for (int ii = 0; ii < 3 * 3; ++ii) {
460+
EXPECT_LT(fabs(virial[ii] - expected_tot_v[ii]), EPSILON);
472461
}
473462
for (int ii = 0; ii < natoms; ++ii) {
474463
EXPECT_LT(fabs(atom_ener[ii] - expected_e[ii]), EPSILON);
475464
}
476-
if (atom_vir.empty()) {
477-
return;
478-
}
465+
EXPECT_FALSE(atom_vir.empty()) << "Atomic virial should not be empty";
479466
EXPECT_EQ(atom_vir.size(), natoms * 9);
480467
for (int ii = 0; ii < natoms * 9; ++ii) {
481468
EXPECT_LT(fabs(atom_vir[ii] - expected_atom_v[ii]), EPSILON);

source/api_cc/tests/test_deeppot_tf_spin.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -381,7 +381,7 @@ TYPED_TEST(TestInferDeepSpinNopbc, cpu_lmp_nlist_atomic) {
381381
EXPECT_EQ(force_mag.size(), natoms * 3);
382382
EXPECT_EQ(virial.size(), 9);
383383
EXPECT_EQ(atom_ener.size(), natoms);
384-
EXPECT_EQ(atom_vir.size(), natoms * 9);
384+
EXPECT_EQ(atom_vir.size(), (natoms + 2) * 9);
385385

386386
EXPECT_LT(fabs(ener - expected_tot_e), EPSILON);
387387
for (int ii = 0; ii < natoms * 3; ++ii) {
@@ -394,7 +394,7 @@ TYPED_TEST(TestInferDeepSpinNopbc, cpu_lmp_nlist_atomic) {
394394
for (int ii = 0; ii < natoms; ++ii) {
395395
EXPECT_LT(fabs(atom_ener[ii] - expected_e[ii]), EPSILON);
396396
}
397-
for (int ii = 0; ii < natoms * 9; ++ii) {
397+
for (int ii = 0; ii < (natoms + 2) * 9; ++ii) {
398398
EXPECT_LT(fabs(atom_vir[ii] - expected_v[ii]), EPSILON);
399399
}
400400
}

source/lmp/tests/test_lammps_spin.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -306,9 +306,10 @@ def test_pair_deepmd_virial(lammps) -> None:
306306
lammps.variables[f"pressure{jj}"].value
307307
) / constants.nktv2p == pytest.approx(-expected_v[:, jj].sum(axis=0) / vol)
308308
for ii in range(9):
309+
jj = [0, 4, 8, 3, 6, 7, 1, 2, 5][ii]
309310
assert np.array(
310-
lammps.variables[f"virial{ii}"].value
311-
) / constants.nktv2p == pytest.approx(expected_v[idx_map, ii])
311+
lammps.variables[f"virial{jj}"].value
312+
) / constants.nktv2p == pytest.approx(expected_v[idx_map, jj])
312313

313314

314315
def test_pair_deepmd_model_devi(lammps) -> None:

0 commit comments

Comments
 (0)