Bug #3397

Gmxapi stopsignaler test is too strict

Added by Pascal Merz 6 months ago. Updated 6 months ago.

Target version:
Affected version - extra info:
Affected version:


Setting a stop signal using gmxapi is done by setting the stophandler signal `stopAtNextNSStep` during force evaluation, and setting the neighborsearching frequency to 1. To test this functionality, the gmxapi stopsignaller test sets the signal during step 0, and checks how many times forces get evaluated in total. It expects that forces are only called twice. This test has two issues.

First, not wrong but confusing, the check is done by comparing EXPECT_EQ(1, restraint->numberOfTimesCalled()); (src/api/cpp/tests/stopsignaler.cpp:232). The naming of numberOfTimesCalled() is confusing, as it returns 0 if it has been called once, 1 if it has been called twice, etc. It is effectively returning the last step is has been called (src/api/cpp/tests/stopsignaler.cpp:162). Renaming this function would make the test easier to understand

Second, the test is too strict. It works with the legacy implementation, because the stop signal happens to be checked between the force evaluation and the global communication. It will hence communicate the signal on the same step it is set, and define the next step as the last step. In modular simulator, however, the stop signal gets checked at the beginning of the step. The signal will hence be communicated on the following step, and the simulation stopped two steps after the signal was set. StopSignalHandler is not promising to stop the simulation on the neighbor searching step after it has been set (and hence communicating the signal on the step is has been set), it is only promising to stop the simulation on the neighbor searching step following the step on which it has communicated the signal. The correct test should hence be EXPECT_LE(2, restraint->numberOfTimesCalled());.

Associated revisions

Revision b0ad9f30 (diff)
Added by Pascal Merz 6 months ago

Loosen gmxapi stopsignaler test

Gmxapi stopsignaler test was checking whether simulations were stopped
on the step after a stop signal was set. This is true for the legacy
implementation, but only due to the exact order of instructions. The
StopSignalHandler does not guarantee to stop simulations at the next
NS step after a signal has been set, it promises to stop a simulation
at the next NS step after a signal has been communicated. This change
loosens the criterion to reflect this.

Further, this change
  • Changes the numberOfTimesCalled() function to timeElapsedSinceStart(),
    also including a different treatment of the call before the restraint
    is ever called, to avoid misunderstandings.
  • Adds a comment to explain the expectations validated by this test.

Fixes #3397

Change-Id: I2a3805a14c03d0ee12ebd21b5863c5243b1c3671


#1 Updated by Anonymous 6 months ago

  • Status changed from Fix uploaded to Resolved

#2 Updated by Paul Bauer 6 months ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF