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

Fix LBGPU velocity interpolation close to boundary #3593

Merged
merged 8 commits into from
Mar 26, 2020

Conversation

RudolfWeeber
Copy link
Contributor

Fixes #

Description of changes:
LB GPu velocity interpolation used incorrect velocities at boundary nodes. This lead to incorrect resutls for interpolations closer than one lattice constant to the boundary.
This adds a missing unit conversion 1/lattice_speed for boundary nodes, which fixes the issue.

Tests for interpolation at the boundary position and at the mid-point between the boundary and the next lattice site.

This should go into 4.1.3

@RudolfWeeber RudolfWeeber requested a review from jngrad March 25, 2020 11:04
@codecov
Copy link

codecov bot commented Mar 25, 2020

Codecov Report

Merging #3593 into python will increase coverage by <1%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           python   #3593    +/-   ##
=======================================
+ Coverage      88%     88%   +<1%     
=======================================
  Files         527     527            
  Lines       23572   23572            
=======================================
+ Hits        20755   20761     +6     
+ Misses       2817    2811     -6
Impacted Files Coverage Δ
src/core/electrostatics_magnetostatics/p3m.cpp 85% <0%> (ø) ⬆️
src/core/polymer.cpp 98% <0%> (+5%) ⬆️

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 bf30a6c...ecaedad. Read the comment docs.

testsuite/python/lb_interpolation.py Outdated Show resolved Hide resolved
testsuite/python/lb_interpolation.py Outdated Show resolved Hide resolved
# np.testing.assert_almost_equal(self.lbf.get_interpolated_velocity(pos)[2], 0.0)
# Check interpolated vel at upper boundary. The node position is at
# box_l[0]-agrid/2.
np.testing.assert_allclose(self.lbf.get_interpolated_velocity(
Copy link
Member

Choose a reason for hiding this comment

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

do we need to guard the result of get_interpolated_velocity with np.copy()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We let the CI decide.

@jngrad
Copy link
Member

jngrad commented Mar 25, 2020

Did you figure out the source of the error Lattice velocity exceeds the Mach number limit in function void lb_boundary_mach_check() in failed CI jobs?

@RudolfWeeber
Copy link
Contributor Author

I don't understand the larger error for the clang:6.0 and cuda 9 images. On my machine, (and apparently on the cuda 10 container), the accuracy is much better. Atol=1E-10 worked locally.

I'm increasing the tolerance for now, because the result is still much better than the one with the missing prefactor without this PR. But I don't understand why that should be needed.

@RudolfWeeber RudolfWeeber force-pushed the lb_interpol_boundary branch from 210b513 to b426d5c Compare March 26, 2020 14:00
@RudolfWeeber
Copy link
Contributor Author

Accuracy reduction was caused by an unintended parameter change (TAU) which I reverted now. still don't understand the platform-dependence, now.

@fweik fweik added this to the Espresso 4.1.3 milestone Mar 26, 2020
@jngrad
Copy link
Member

jngrad commented Mar 26, 2020

Seems to be working now. Could you please revert the print/sys.stdout changes and squash the PR down to a single commit? This will make backporting go smoother.

@RudolfWeeber
Copy link
Contributor Author

RudolfWeeber commented Mar 26, 2020 via email

@jngrad jngrad added the automerge Merge with kodiak label Mar 26, 2020
@kodiakhq kodiakhq bot merged commit 0479f39 into espressomd:python Mar 26, 2020
@KaiSzuttor
Copy link
Member

is the boundary velocity stored in MD units? The correct fix would be to store it in LB units and not adding this unit conversion in the LB code. This should be reverted.

@RudolfWeeber
Copy link
Contributor Author

RudolfWeeber commented Mar 30, 2020 via email

@KaiSzuttor
Copy link
Member

all right

RudolfWeeber pushed a commit to RudolfWeeber/espresso that referenced this pull request Jul 4, 2020
Fixes #

Description of changes:
LB GPu velocity interpolation used incorrect velocities at boundary nodes. This lead to incorrect resutls for interpolations closer than one lattice constant to the boundary.
This adds a missing unit conversion 1/lattice_speed for boundary nodes, which fixes the issue.

Tests for interpolation at the boundary position and at the mid-point between the boundary and the next lattice site.

This should go into 4.1.3
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.

4 participants