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

LB+particles: guard against missed coupling due to round-off error #4827

Merged
merged 4 commits into from
Dec 5, 2023

Conversation

RudolfWeeber
Copy link
Contributor

@RudolfWeeber RudolfWeeber commented Nov 24, 2023

Fixes #4825

Description of changes:

  • Bugfix: particles outside the simulation box are now properly coupled using PBC. The coordinates are folded before considering shifted positions in the LB particle coupling code. Also, a test that fails without the fix is added. To my understanding, if the particle position is folded, and then all combination of folded_pos[i] +/- box_l[i] for all Cartesian directions are considered, both the particle in the primary box and all potential halo regions are caught.

@RudolfWeeber
Copy link
Contributor Author

in algorithsm/periodic_fold.hpp, first the lower, then the upper limit is checked, so here
-epsilon + box_l = box_l (due to round-off error)
but box_l >= box_l so
folded_pos = box_l - box_l =0

Let's consider the other case:
box_l +epislon is folded to
box_l +epsilon -box_l
even if this evaluates to 0 (rather than epsilon) due to rounding, the resulting coordinate is still folded, ie., 0<=x<box_l

box_l-epsilon is < box_l so the folding code does nothing, the coordinate is already folded.

So as I understand it, Algorithm::periodic_fold() is correct in all cases. Can someone please double check this?

Copy link
Contributor

@christophlohrmann christophlohrmann left a comment

Choose a reason for hiding this comment

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

I agree with the code changes and all arguments made.
Still, a cpp unit test for algorithm/periodic_fold that explicitly tests the cases mentioned in @RudolfWeeber's comment would support the argument better than my agreement.
Also the python test could look not only at the fact that the particle is coupled, but also could check that the particle is coupled correctly, i.e. momentum conserving.

@jngrad
Copy link
Member

jngrad commented Dec 5, 2023

@RudolfWeeber's argument on the periodic folding w.r.t. roundoff errors is supported by the C++ unit tests:

/* Corner left */
{
auto const x = std::nextafter(0., -1.);
auto const box = 10.;
auto const res = periodic_fold(x, 0, box);
BOOST_CHECK(res.first >= 0.);
BOOST_CHECK(res.first <= box);
BOOST_CHECK(std::abs(res.first - x + res.second * box) <=
4. * std::numeric_limits<double>::epsilon());
}

Here res.first (position) is 0.0 and res.second (image box count) is 0, so the folding was applied twice. This test also checks for the case box+epsilon, and for a non-zero image box counter.

@jngrad jngrad added this to the ESPResSo 4.2.2 milestone Dec 5, 2023
@jngrad jngrad added Core BugFix automerge Merge with kodiak labels Dec 5, 2023
@kodiakhq kodiakhq bot merged commit a22c2fe into espressomd:python Dec 5, 2023
10 checks passed
jngrad pushed a commit to jngrad/espresso that referenced this pull request Dec 5, 2023
…spressomd#4827)

Fixes espressomd#4825

Description of changes:
* Bugfix: particles outside the simulation box are now properly coupled using PBC. The coordinates are folded before considering shifted positions in the LB particle coupling code. Also, a test that fails without the fix is added. To my understanding, if the particle position is folded, and then all combination of `folded_pos[i]` +/- `box_l[i]` for all Cartesian directions are considered, both the particle in the primary box and all potential halo regions are caught.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LB: Particle not coupled due to rounding error
3 participants