Recalc cutoff on skin change

[KonradBreitsprecher]

• Corrected wrong ifdef p3m for mmm2d in coulomb.cpp
• In event.cpp: For mmm2d, the cutoff depends on the skin (see Coulomb::cutoff()).
  A skin change so far did not respect this from what I saw.

[fweik]

I don't think any of this is actually needed, can you please check if it's sufficient to just set the curoff to a finit constant? I think this just in place so that the short-range loop is not skipped, so any finite value, or potentially 0. will do. @RudolfWeeber do you remember? Also this needs a test to check the behavior.

[codecov]

Codecov Report

Merging #3003 into python will decrease coverage by <1%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           python   #3003   +/-   ##
======================================
- Coverage      83%     83%   -1%     
======================================
  Files         530     530           
  Lines       26100   26100           
======================================
- Hits        21915   21914    -1     
- Misses       4185    4186    +1

Impacted Files	Coverage Δ
src/core/electrostatics_magnetostatics/coulomb.cpp	79% <100%> (ø)	⬆️
src/core/electrostatics_magnetostatics/p3m.cpp	85% <0%> (-1%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2948859...c53e6ad. Read the comment docs.

[fweik]

Maybe I am confusing this with ELC, but it still does not make sens: the maximal cutoff is used to calculate layer_h, so there is a circular dependency here. Also I fail to see how any of this is related to the skin. This is all very confused.

[RudolfWeeber]

My understanding is that the mmm2d kernel is executed for all pairs in a layer. As Flo said, the cutoff just has to be a positive epsilon, otherwise the short range loop is skipped entirely. I’d only change that, if it actually can be shown that it fixes something.

[fweik]

What the method actually needs is cutoff infinity in x and y direction, and none in z direction (This would also force a compatible cell system, as this can only be supported by n2 and layered). This can not be expressed because we only consider a scalar cutoff. The hackish solution to this implemented is to set the cutoff to an arbitrary finite value, and have layered not check the value. So I think you should set it to std::max(box_l[0], box_l[1]) to make the cutoff incompatible with dd.
(What it really should be is {inf, inf, INACTIVE}).

[KonradBreitsprecher]

Ok using a small eps for MMM2D in Coulomb:cutoff cured my MWE:

from __future__ import print_function
import espressomd
import numpy as np
import espressomd.electrostatics
from espressomd import electrostatic_extensions
box_l = 10.0
system = espressomd.System(box_l=[box_l]*3)
system.time_step = 0.01
system.cell_system.set_layered(n_layers=20, use_verlet_lists=False)
system.periodicity = [1, 1, 0]
system.part.add(pos=(1, 1, 1), q=1)
system.part.add(pos=(9, 9, 9), q=-1)
mmm2d = espressomd.electrostatics.MMM2D(prefactor=1.0, maxPWerror=1e-3)
system.actors.add(mmm2d)
system.cell_system.skin=0.4

Without the PR I got:
ERROR: layered: maximal interaction range 0.9 larger than layer height 0.5
fired by cells_on_geometry_change() which is now gone.

[RudolfWeeber]

bors r+

[bors]

👎 Rejected by code reviews

[fweik]

There is still no test.

[RudolfWeeber]

On Tue, Aug 06, 2019 at 01:08:42AM -0700, Florian Weik wrote: There is still no test.

Sry. Overlooked that. @KonradBreitsprecher?

[RudolfWeeber]

@KonradBreitsprecher, could you please provide a test, then this can go into 4.1.

[KonradBreitsprecher]

I'm not getting the error anymore for the python branch (maximal interaction range larger than layer height), sth else has fixed that. Not sure what to test, for now I added a test that checks if mm2d changes the max_cut from zero to nonzero and does a test integration.

[RudolfWeeber]

Do I understand correctly that this is no longer needed?

[fweik]

Aye

…

On Thu, Sep 12, 2019, 11:30 RudolfWeeber ***@***.***> wrote: Do I understand correctly that this is no longer needed? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3003?email_source=notifications&email_token=AAG2FXZ5N6SH4NSRRVSPK53QJID4DA5CNFSM4IGOP4B2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6RIQCY#issuecomment-530745355>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAG2FX26R3F6NOIUVMKKYC3QJID4DANCNFSM4IGOP4BQ> .