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

Refactor particle management #4605

Merged
merged 3 commits into from
Nov 11, 2022

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented Nov 8, 2022

Description of changes:

  • particle state tracking is now fully encapsulated
    • helps with separation of concerns and reducing code duplication
  • particle creation and particle moves are now carried out by 2 independent functions

jngrad and others added 3 commits November 8, 2022 13:03
Encapsulate code responsible for keeping track of reacted particles
and restoring the original state. Factor out code duplication.
The test case re-uses the reaction_ensemble_complex_reaction.py sample.

Co-authored-by: Jonas Landsgesell <[email protected]>
@jngrad jngrad force-pushed the refactor_particle_management branch from be145b2 to ae2b400 Compare November 8, 2022 15:31
@jngrad jngrad added the Core label Nov 8, 2022
@jngrad jngrad requested review from pm-blanco and reinaual November 8, 2022 16:29
@pm-blanco
Copy link
Contributor

@jngrad Maybe I did not understand properly what you meant in this PR but it does not seem to me that it actually throws an error when reacting particles of the wrong type. I tested your PR with the script that I provide in #4595 and it still does not show any error when producing the bug.

@jngrad
Copy link
Member Author

jngrad commented Nov 11, 2022

@jngrad Maybe I did not understand properly what you meant in this PR but it does not seem to me that it actually throws an error when reacting particles of the wrong type. I tested your PR with the script that I provide in #4595 and it still does not show any error when producing the bug.

Here is the output on my workstation:

jgrad@seraue:/work/jgrad/espresso-fork-PR/build-lama-release2$ mpirun -n 1 ./pypresso mwe_pablo.py 
step  0
Before widom: a: 1 H: 1 Ha: 1 Na: 2 Cl: 2
total charge  0.0
/work/jgrad/espresso-fork-PR/build-lama-release2/src/python/espressomd/reaction_methods.py:462:
FutureWarning: arguments 'reactant_coefficients' and 'product_coefficients' are deprecated and
are no longer necessary for the constant pH ensemble. They are kept for backward compatibility
but might be deleted in future versions.
  warnings.warn(warn_msg, FutureWarning)
Traceback (most recent call last):
  File "mwe_pablo.py", line 105, in <module>
    widom.calculate_particle_insertion_potential_energy(reaction_id=0)
  File "/work/jgrad/espresso-fork-PR/build-lama-release2/src/python/espressomd/reaction_methods.py", line 515, in calculate_particle_insertion_potential_energy
    return self.call_method(
  File "script_interface.pyx", line 159, in espressomd.script_interface.PScriptInterface.call_method
RuntimeError: Invalid local particle index entry.

In the end, the bug needs to be removed with your PR.

@pm-blanco
Copy link
Contributor

pm-blanco commented Nov 11, 2022

That's strange, somehow I cannot reproduce it in my workstation. I get

mpirun -n 1 ../../espresso_jn/build/pypresso  bug_widom.py 
/home/pablo/espresso_jn/build/src/python/espressomd/reaction_methods.py:462: FutureWarning: arguments 'reactant_coefficients' and 'product_coefficients' are deprecated and are no longer necessary for the constant pH ensemble. They are kept for backward compatibility but might be deleted in future versions.
  warnings.warn(warn_msg, FutureWarning)
step  0
Before widom: a: 2 H: 2 Ha: 0 Na: 2 Cl: 2
total charge  0.0
After widom: a: 2 H: 1 Ha: 0 Na: 2 Cl: 2
total charge  -1.0

In the end, the bug needs to be removed with your PR.

it is quite right that we need to solve it in another PR but, just to avoid confusion, my current PR is just on the refactoring of the MC function to move particles. I was waiting for your green light on my idea of checking the bookeeping list each time the do_reaction function is called as you mentioned that you wanted to discuss it with Rudolf in my issue, since it comes with an associated performance loss.

Anyway, I will not oppose to merge this PR for this error message since I agree with you that we need to solve the bug in another PR anyway.

@jngrad jngrad added the automerge Merge with kodiak label Nov 11, 2022
@kodiakhq kodiakhq bot merged commit 847c96e into espressomd:python Nov 11, 2022
@jngrad jngrad deleted the refactor_particle_management branch November 11, 2022 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge with kodiak Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants