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

P3M-based electrostatics and magnetostatics tuning functions ignore errors #3868

Closed
jngrad opened this issue Aug 19, 2020 · 5 comments · Fixed by #3869
Closed

P3M-based electrostatics and magnetostatics tuning functions ignore errors #3868

jngrad opened this issue Aug 19, 2020 · 5 comments · Fixed by #3869
Assignees
Labels

Comments

@jngrad
Copy link
Member

jngrad commented Aug 19, 2020

The electrostatics tuning function runs into an infinite loop if the system time step is not set. Magnetostatics tuning is not affected. Regression introduced in 4.1.0 by #3141. MWE:

import espressomd
from espressomd import electrostatics
system = espressomd.System(box_l=3*[1])
system.part.add(pos=[[0, 0, 0], [.5, .5, .5]], q=[-1, 1])
# system.time_step = 0.01
solver = electrostatics.P3M(prefactor=2, accuracy=1e-2)
system.actors.add(solver)  # infinite loop
@jngrad jngrad added the Bug label Aug 19, 2020
@jngrad jngrad added this to the Espresso 4.1.4 milestone Aug 19, 2020
@jngrad jngrad self-assigned this Aug 19, 2020
@jngrad
Copy link
Member Author

jngrad commented Aug 19, 2020

There are more issues with electrostatics tuning: several errors are simply appended to the standard log string, preventing handle_errors() from halting the flow of the program and leaving the system in an undefined state. Magnetostatics tuning is also affected.

@jngrad jngrad changed the title Electrostatics tuning ignores runtimeErrorMsg P3M-based electrostatics and magnetostatics tuning functions ignore errors Aug 19, 2020
@jngrad
Copy link
Member Author

jngrad commented Aug 19, 2020

The magnetostatics dp3m_rtbisection() function doesn't throw on error, and the error code it returns isn't checked, so we end up in an infinite loop...

@jngrad
Copy link
Member Author

jngrad commented Aug 19, 2020

The script interface for P3M in the metallic case is broken since before 4.0.

@jngrad
Copy link
Member Author

jngrad commented Aug 19, 2020

The code samples in the user guide for electrostatics/magnetostatics don't run in pypresso. The user guide says P3M requires a Bjerrum length when it actually requires a prefactor since 4.1. The user guide says using DipolarP3M requires reading the P3M literature but doesn't provide the list of P3M citations.

@jngrad
Copy link
Member Author

jngrad commented Aug 20, 2020

The epsilon parameter is changed to 0 during tuning in P3M and P3MGPU classes since ESPResSo 4.0.0. DipolarP3M is not affected.

@kodiakhq kodiakhq bot closed this as completed in #3869 Aug 24, 2020
kodiakhq bot added a commit that referenced this issue Aug 24, 2020
Fixes #3868

Description of changes:
- core: correctly propagate errors codes in the P3M/DP3M tuning functions to avoid infinite loops
- core: rely on `runtimeErrorMsg()` instead of printing error messages to stderr or char arrays
- python: catch errors from the P3M/DP3M tuning functions in the `tune()` method of the relevant python Actors
- python: fix the broken `'metallic'` case of the P3M `epsilon` parameter
- testsuite: add tests for the `'metallic'` case and for the new exception mechanism
- python: fix the non-metallic epsilon case (epsilon was always set to 0 for `P3M ` and `P3MGPU` in the core since 4.0.0)
- documentation: fix broken code examples in the user guide
- documentation: mention P3M doesn't support non-cubic boxes for non-metallic epsilon
jngrad pushed a commit to jngrad/espresso that referenced this issue Aug 25, 2020
…#3869)

Fixes espressomd#3868

Description of changes:
- core: correctly propagate errors codes in the P3M/DP3M tuning functions to avoid infinite loops
- core: rely on `runtimeErrorMsg()` instead of printing error messages to stderr or char arrays
- python: catch errors from the P3M/DP3M tuning functions in the `tune()` method of the relevant python Actors
- python: fix the broken `'metallic'` case of the P3M `epsilon` parameter
- testsuite: add tests for the `'metallic'` case and for the new exception mechanism
- python: fix the non-metallic epsilon case (epsilon was always set to 0 for `P3M ` and `P3MGPU` in the core since 4.0.0)
- documentation: fix broken code examples in the user guide
- documentation: mention P3M doesn't support non-cubic boxes for non-metallic epsilon
jngrad pushed a commit to jngrad/espresso that referenced this issue Aug 27, 2020
…#3869)

Fixes espressomd#3868

Description of changes:
- core: correctly propagate an error code in the P3M tuning function to avoid an infinite loop
- python: fix the broken `'metallic'` case of the P3M `epsilon` parameter
- python: fix the non-metallic epsilon case (epsilon was always set to 0 for `P3M ` and `P3MGPU` in the core since 4.0.0)
- documentation: fix broken code examples in the user guide
- documentation: mention P3M doesn't support non-cubic boxes for non-metallic epsilon
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 a pull request may close this issue.

1 participant