Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New integration tests framework #3999

Merged
merged 24 commits into from
Nov 24, 2020
Merged

New integration tests framework #3999

merged 24 commits into from
Nov 24, 2020

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented Nov 19, 2020

Fixes #3883

Description of changes:

  • write more thorough RNG checks for thermostats
  • skip slow/statistical tests in coverage and sanitizer builds
  • fix broken ifdefs for Brownian Dynamics
  • cleanup IBM code

jngrad and others added 14 commits November 19, 2020 16:15
Fix a few issues in the Langevin RNG check.
Move checks with long runtime into separate files.
Test backported from 4.3, with slight changes to the system.time_step
and to error tolerances. The test runs faster thanks to the smaller
number of integration steps in the IBM checks.

Co-authored-by: Rudolf Weeber <[email protected]>
Move the elastic forces descriptions from the advanced methods page
to the bonded interactions page. Mention which figures in kruger12a
provide schematics for the bonds. Update doxygen comments.
The per-particle temperature, gamma and gamma_rot must be defined
if BROWNIAN_PER_PARTICLE is enabled.
Check the RNG with ROTATION and per-particle gamma/temperature.
Use similar filenames and class structures. Add the missing call to
`acc.finalize()` for Correlator objects. Check pickling of the
MeanVariance object.
This checkpointing function was only used in the Tcl interface.
The alternative code was never used. Variable names have diverged.
Exclude statistical tests (e.g. thermostat tests where velocity
distributions require a lot of sampling) and deterministic tests
with long runtime from CI jobs where coverage or sanitizers are
enabled.
@jngrad jngrad marked this pull request as ready for review November 19, 2020 17:34
@jngrad
Copy link
Member Author

jngrad commented Nov 19, 2020

Here is the summary for the python testsuite, using the same CI runner when possible:

CI job Old runtime New runtime speed-up
clang-sanitizer 2109.2s 988.1s 19m (53%)
no_rotation 647.7s 468.5s 3m (28%)
maxset 961.6s 616.0s 6m (36%)
default 970.5s 771.3s 3m (20%)
cuda11-maxset 1113.4s 774.9s 6m (30%)

The complete pipeline runtime went from 72 min to 38 min. The Clang job completes in 35 min instead of 50 min.

@RudolfWeeber your plan in #3883 (comment) was really good, however I had to deviate from it slightly:

  • virtual_sites_tracers is needed to get code coverage for IBM particle coupling to the fluid (not sure why though, the LB actor is removed before the IBM bonds are created)
  • Brownian/NpT were moved fully, but new files with minimum checks were introduced for both integrators, otherwise their code coverage would be 0%

Other remarks:

  • make check_python still works the same way as before
  • make check_python_skip_long to run the subset of fast tests
  • the comment lines in the thermostat tests about RNG seed and counter state were obtained using cout.diff

@jngrad jngrad added this to the Espresso 4.2 milestone Nov 19, 2020
Also skip the slow diffusion and trajectory tests in coverage builds.
Write checks for NpT integrator. Move checks for SD integrator
to a test file that isn't skipped in coverage jobs.
@jngrad
Copy link
Member Author

jngrad commented Nov 20, 2020

The slowest tests are now:

  • stokesian_dynamics: 1 min 10 sec in sanitizer, 40 sec in coverage, 20-30s in others
    • now skipped in sanitizer/coverage builds
    • new test added to check the SD interface
  • elc_vs_analytic: 1 min 20 sec in sanitizer/coverage
    • removing it from coverage builds would require writing a small test without constant potential to preserve code coverage
  • reaction ensemble tests:
    • constant_pH: 1 min 30 sec in sanitizer, 45 sec in coverage
    • reaction_ensemble: 30-40 sec in sanitizer/coverage, 5 sec in others
    • widom_insertion: 30-40 sec in sanitizer/coverage, 5 sec in others
    • wang_landau_reaction_ensemble: already skipped in sanitizer/coverage
    • long-term plan would be to skip these tests in sanitizer/coverage and write much smaller tests to check the RE interface (error messages, output files, and run a few reaction steps), like I did here for the thermostats and integrators

This reverts partially commit 51f30d5.
@@ -35,8 +35,8 @@ class ElcTest(ut.TestCase):
def test_finite_potential_drop(self):
s = self.system

p1 = s.part.add(pos=GAP + [0, 0, 1], q=+1)
p2 = s.part.add(pos=GAP + [0, 0, 9], q=-1)
p1 = s.part.add(pos=[0, 0, 1], q=+1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it doesn't matter, where in the system the gap is? @reinaual
Did this change influence the test outcome?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change did not alter the numerical result. I think the gap is always at the higher value of the z-axis:

/** Up to where particles can be found. */
double h;

elc_params.h = box_geo.length()[2] - gap_size;

If particles enter the gap, they are misclassified as particles from the dielectric contrast when calculating the forces and energies. The documentation warns against using ELC without walls to block particles from entering the gap:

* The method relies on a slab of the simulation box perpendicular to the
z-direction not to contain particles. The size in z-direction of this slab
is controlled by the ``gap_size`` parameter. The user has to ensure that
no particles enter this region by means of constraints or by fixing the
particles' z-coordinate. When there is no empty slab of the specified size,
the method will silently produce wrong results.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory it should not matter, to my knowledge in the implementation it does. The lower interface is assumed at z=0 and the upper one at z=h, which is relevant for the error-estimation and the placement of the image charges.
To my understanding the testcase should be correct for every given prefactor, however setting it to 1e-100 allows to explicitly test the potential difference contribution but hides the misplaced particles in this case.

@jngrad jngrad added the automerge Merge with kodiak label Nov 24, 2020
@kodiakhq kodiakhq bot merged commit 32a52a5 into espressomd:python Nov 24, 2020
@jngrad jngrad deleted the fix-3883 branch December 14, 2020 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Framework for statistical tests
3 participants