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

ELC core object can have uninitialized data members #4230

Closed
jngrad opened this issue Apr 16, 2021 · 0 comments · Fixed by #4231
Closed

ELC core object can have uninitialized data members #4230

jngrad opened this issue Apr 16, 2021 · 0 comments · Fixed by #4231
Assignees

Comments

@jngrad
Copy link
Member

jngrad commented Apr 16, 2021

The ELC actor is set up by recycling the old struct and assigning new values to individual members, which is a potential source of bugs. For example, when the setter is called with const_pot=false, the core parameter elc_params.const_pot is not updated:

if (const_pot) {
elc_params.const_pot = true;
elc_params.pot_diff = pot_diff;
}

If the user deleted an older ELC actor that was set with const_pot=true, the new actor has constant potential enabled. MWE:

import espressomd.electrostatics

system = espressomd.System(box_l=[10, 10, 13], time_step=0.01)
system.cell_system.skin = 0.0
system.part.add(pos=[(0, 0, 1), (0, 0, 9)], q=[1, -1])

p3m = espressomd.electrostatics.P3M(prefactor=1, mesh=32, cao=5, r_cut=0.9570,
                                    alpha=2.272731, tune=False, accuracy=1e-3)
elc_params = {'p3m_actor': p3m, 'gap_size': 3., 'maxPWerror': 1e-3,
              'delta_mid_top': -1, 'delta_mid_bot': -1, 'pot_diff': -3.}

elc = espressomd.electrostatics.ELC(const_pot=True, **elc_params)
system.actors.add(elc)
print(elc.get_params()['const_pot'])  # prints True

system.actors.clear()

elc = espressomd.electrostatics.ELC(const_pot=False, **elc_params)
system.actors.add(elc)  # should fail: metallic boundaries
print(elc.get_params()['const_pot'])  # prints True instead of False

Output:

True
True

Expected:

True
Traceback (most recent call last):
  File "mwe.py", line 19, in <module>
    system.actors.add(elc)  # should fail: metallic boundaries
  File "actors.pyx", line 215, in espressomd.actors.Actors.add
  File "actors.pyx", line 70, in espressomd.actors.Actor._activate
  File "electrostatics.pyx", line 533, in espressomd.electrostatics.ELC._activate_method
  File "electrostatics.pyx", line 516, in espressomd.electrostatics.ELC._set_params_in_es_core
RuntimeError: ELC with two parallel metallic boundaries requires the const_pot option

Older versions of ESPResSo are not affected by this bug, because before 4.2 the ELC setter could only be called once.

In addition, when resizing the box in the z-direction, the gap parameter h is not updated, which leads to incorrect ELC energies being calculated and particles entering the gap region without triggering an exception:

import espressomd.electrostatics

system = espressomd.System(box_l=[10, 10, 13], time_step=0.01)
system.cell_system.skin = 0.0
system.part.add(pos=[(0, 0, 1), (0, 0, 9)], q=[1, -1])

p3m = espressomd.electrostatics.P3M(prefactor=1, mesh=32, cao=5, r_cut=0.9570,
                                    alpha=2.272731, tune=False, accuracy=1e-3)
elc_params = {'p3m_actor': p3m, 'gap_size': 3., 'maxPWerror': 1e-3,
              'delta_mid_top': -1, 'delta_mid_bot': -1, 'pot_diff': -3.}

elc = espressomd.electrostatics.ELC(const_pot=True, **elc_params)
system.actors.add(elc)
system.box_l = [10, 10, 10] # particle 2 has now entered the gap
system.analysis.energy()['coulomb']
system.part[1].pos = [0, 0, 6]
print('E =', system.analysis.energy()['coulomb'])

Output:

E = -1.6331453454189522

Expected:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "analyze.pyx", line 474, in espressomd.analyze.Analysis.energy
  File "utils.pyx", line 236, in espressomd.utils.handle_errors
  File "utils.pyx", line 255, in espressomd.utils.handle_errors
Exception: calc_long_range_energies failed: ERROR: Particle 1 entered ELC gap region by 2
E = -2.345896134603552
@jngrad jngrad added the Bug label Apr 16, 2021
@jngrad jngrad added this to the Espresso 4.2 milestone Apr 16, 2021
@jngrad jngrad self-assigned this Apr 16, 2021
@jngrad jngrad changed the title ELC core object can have undefined fields ELC core object can have uninitialized data members Apr 16, 2021
@jngrad jngrad added the Core label Apr 16, 2021
@kodiakhq kodiakhq bot closed this as completed in #4231 Apr 16, 2021
kodiakhq bot added a commit that referenced this issue Apr 16, 2021
Fixes #4230

Description of changes:
- create a new ELC struct every time a new ELC actor is set up (#4220)
- use standard exception mechanism in the ELC setter (#4219)
- ELC data members are updated upon change to the system
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.

1 participant