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

MMM1DGPU: Segmentation fault without correct periodicity #4056

Closed
reinaual opened this issue Dec 17, 2020 · 4 comments · Fixed by #4064
Closed

MMM1DGPU: Segmentation fault without correct periodicity #4056

reinaual opened this issue Dec 17, 2020 · 4 comments · Fixed by #4064
Assignees

Comments

@reinaual
Copy link
Contributor

reinaual commented Dec 17, 2020

adding MMM1DGPU to the system without providing the correct periodicity results in a segfault.

import numpy as np
from espressomd import System, electrostatics

system = System(box_l = [1, 1, 1])
system.time_step = 0.01
system.cell_system.skin = 0.4
# system.cell_system.set_n_square()
# system.periodicity = (0, 0, 1)

mmm1d = electrostatics.MMM1DGPU(prefactor = 1, maxPWerror = 1e-2)

system.part.add(pos=(0.5, 0.5, 0.75), q=1)
system.part.add(pos=(0.5, 0.5, 0.25), q=-1)

system.actors.add(mmm1d)
@jngrad
Copy link
Member

jngrad commented Dec 17, 2020

It's just a missing except+, although looking at the cuda code, we could probably replace a lot of std::cerr by throw, and more importantly, add python tests to check for these exceptions.

@jngrad jngrad self-assigned this Dec 17, 2020
@jngrad
Copy link
Member

jngrad commented Dec 17, 2020

As it turns out, MMM1D on GPU was never tested in CI, because the GPU class is a verbatim copy of the CPU class: they both import the CPU actor.

@jngrad
Copy link
Member

jngrad commented Dec 17, 2020

offline discussion: the exception mechanism for MMM1D CPU and GPU is quite tangled in the core. Also, it depends on #4020. The test doesn't pass for MMM1D GPU due to large deviations from the analytical solution. We should benchmark MMM1D on GPU to see if it's worth investing more time into fixing it, otherwise we'll remove it in 4.2.

@jngrad
Copy link
Member

jngrad commented Dec 21, 2020

MMM1D with 1000 particles and 500 time steps: 60s on CPU with 4 cores, 1s on GPU. ScaFaCoS, CPU and GPU implementations agree on forces up to 1e-6. ScaFaCoS and CPU agree on energy up to 1e-6, while GPU energies are way off.

@kodiakhq kodiakhq bot closed this as completed in #4064 Dec 22, 2020
kodiakhq bot added a commit that referenced this issue Dec 22, 2020
Fixes #4056

Description of changes:
- catch segfaults due to wrong periodicity or cellsystem
- fix MMM1D test case (broken since 4.1.0)
- fix bug in the MMM1D energy function on GPU (introduced in 4.2-dev)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants