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

add ext_force_density to ekin #4203

Merged
merged 4 commits into from
Apr 7, 2021

Conversation

christophlohrmann
Copy link
Contributor

Description of changes:

  • Add interface for external forces on the fluid that ekin passes to lb
  • test together with the lb poiseuille testcase

@RudolfWeeber
Copy link
Contributor

@itischler, could you review this one, since you are familiar with the EK code?

The way EK "remote-controls" LB is really odd, but as I understand it, that's the way it's currently implemented for all EK/LB features.

This will be different, when we switch to Walberla.

I foudn one issue: As I see it, you added EK to the list of required featues for the Poisseuille test. Please avoid that. The pure LB version should run independently of whether or not EK is built in. In addition, the EK+LB-poiseuille test should run if EK is built in. I.e., you need two test funcitons test_lb_poisseuille() and test_lb_ek_poisseuille() and a third funciton, which will contain all the code shared between the two.

@jngrad
Copy link
Member

jngrad commented Apr 6, 2021

I foudn one issue: As I see it, you added EK to the list of required featues for the Poisseuille test. Please avoid that. The pure LB version should run independently of whether or not EK is built in. In addition, the EK+LB-poiseuille test should run if EK is built in. I.e., you need two test funcitons test_lb_poisseuille() and test_lb_ek_poisseuille() and a third funciton, which will contain all the code shared between the two.

I think what he did is correct: a new test class was introduced that is skipped if Ekin is not compiled in. The pure LB still runs independently.

Copy link
Member

@jngrad jngrad left a comment

Choose a reason for hiding this comment

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

minor readability suggestions

testsuite/python/lb_poiseuille.py Outdated Show resolved Hide resolved
src/core/grid_based_algorithms/electrokinetics_cuda.cu Outdated Show resolved Hide resolved
Copy link
Contributor

@itischler itischler left a comment

Choose a reason for hiding this comment

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

Seems good to me.
Just noticed, that modification to external force after the method was activated, would not apply, since thy are not communicated to the gpu. However this is also true to ext forces applied to the species. So this is probably a task for an other pr.

@jngrad jngrad added the Python label Apr 7, 2021
@jngrad jngrad added this to the Espresso 4.2 milestone Apr 7, 2021
Copy link
Member

@jngrad jngrad left a comment

Choose a reason for hiding this comment

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

LGTM

@KaiSzuttor KaiSzuttor added the automerge Merge with kodiak label Apr 7, 2021
@KaiSzuttor
Copy link
Member

Seems good to me.
Just noticed, that modification to external force after the method was activated, would not apply, since thy are not communicated to the gpu. However this is also true to ext forces applied to the species. So this is probably a task for an other pr.

please open a ticket

@kodiakhq kodiakhq bot merged commit 157a048 into espressomd:python Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge with kodiak Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants