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

core: Removed MEMBRANE_COLLISION + out_direction #3418

Merged
merged 4 commits into from
Jan 20, 2020
Merged

core: Removed MEMBRANE_COLLISION + out_direction #3418

merged 4 commits into from
Jan 20, 2020

Conversation

fweik
Copy link
Contributor

@fweik fweik commented Jan 17, 2020

Fixes #2214. Fixes #3416.

Description of changes:

  • Removed membrane collision
  • Removed out_direction

@codecov
Copy link

codecov bot commented Jan 17, 2020

Codecov Report

Merging #3418 into python will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           python   #3418   +/-   ##
======================================
  Coverage      86%     86%           
======================================
  Files         534     534           
  Lines       25227   25227           
======================================
  Hits        21793   21793           
  Misses       3434    3434

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 b6d92c7...08e8f97. Read the comment docs.

@fweik fweik requested a review from RudolfWeeber January 17, 2020 17:17
@fweik fweik changed the title WIP: core: Remved MEMBRANE_COLLISION + out_direction core: Remved MEMBRANE_COLLISION + out_direction Jan 17, 2020
@jngrad
Copy link
Member

jngrad commented Jan 18, 2020

Skimmed through the diff and the changes looked good to me. Can RBCs now cross each other?

@fweik
Copy link
Contributor Author

fweik commented Jan 18, 2020

Can RBCs now cross each other?

Probably. But I'm not sure what to do about this.

Fix the box geometry and correct filename in the docs.
@jngrad
Copy link
Member

jngrad commented Jan 20, 2020

@fweik actually, the simulation crashes as soon as a RBC makes contact with a shape:

ERROR: Constraint violated by particle 3 dist -0.999825
Traceback (most recent call last):
  File "motivation.py", line 151, in <module>
    system.integrator.run(steps=2500)
  File "integrate.pyx", line 60, in espressomd.integrate.IntegratorHandle.run
  File "integrate.pyx", line 196, in espressomd.integrate.Integrator.run
  File "utils.pyx", line 264, in espressomd.utils.handle_errors
Exception: Encountered errors during integrate: ERROR: Constraint violated by particle 3 dist -0.999825

The bug was present before this PR. We might as well update the espresso features table to mark OIF as experimental. The OIF Sphinx documentation was written as a walkthrough for the sample, so deleting the sample would make the docs difficult to follow.

@fweik
Copy link
Contributor Author

fweik commented Jan 20, 2020

@jngrad This is potentially because the interaction with the wall is soft. But I see no point in fixing any of this. For me at least, this is far out of scope.

@jngrad
Copy link
Member

jngrad commented Jan 20, 2020

the interaction with the wall is soft

I used to be able to squeeze RBC through narrow geometries in the past, and they deformed without causing a crash. A regression was introduced at some point, but I won't look into it. This can be merged as is. Even though CI is red, kodiak won't mind and will pull in the pint fix during merging.

@fweik fweik added the automerge Merge with kodiak label Jan 20, 2020
@jngrad jngrad changed the title core: Remved MEMBRANE_COLLISION + out_direction core: Removed MEMBRANE_COLLISION + out_direction Jan 20, 2020
@jngrad jngrad added this to the Espresso 4.2 milestone Jan 20, 2020
@kodiakhq kodiakhq bot merged commit 4bf0f3c into espressomd:python Jan 20, 2020
@fweik fweik deleted the membrane branch January 22, 2020 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge with kodiak
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Out_direction and membrane_collision can be removed membrane_collision is broken
2 participants