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.

Sign up for GitHub

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

Jump to bottom

Make reaction methods run in parallel #4666

Merged
merged 6 commits into from
Feb 24, 2023
Merged

Make reaction methods run in parallel #4666

merged 6 commits into from
Feb 24, 2023

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented Feb 10, 2023 •
edited
Loading

Partial fix for #4614
Partial fix for #4617
Follow-up to #4637

Description of changes:

  • reaction methods are now fully parallel and no longer rely on MpiCallbacks
  • reaction methods are no longer entangled with the particle management code
  • the Monte Carlo acceptance probability is now implemented in Python
  • API changes:
    • it is no longer possible to change the reaction constant of an existing reaction with gamma <= 0
    • ReactionAlgorithm.reaction() now takes steps instead of reaction_steps as argument
    • when setting up a MC method with two or more reactions, a runtime error is raised if a reaction accidentally overwrites the default charge of a specific type with a different value
  • under-the-hood changes:
    • the particle_data.cpp source file was removed
    • the EspressoSystemStandAlone class no longer relies on MpiCallbacks
    • frequency of cell system invalidation during Monte Carlo trial moves has been significantly reduced
    • the new Python interface of reaction methods is around 40% slower than the original C++ implementation

@jngrad @pm-blanco
Make reaction methods run in parallel
801857d 
Rewrite reaction methods without MpiCallbacks. Add sanity checks.
Rewrite some of the functionality as Python code and expose the
`calculate_acceptance_probability()` function in Python to make
prototyping of new Monte Carlo methods easier. Reduce overhead
from the cell system when altering particle properties.

Co-authored-by: Pablo Miguel Blanco Andrés <[email protected]>
@jngrad jngrad force-pushed the parallel-monte-carlo branch 4 times, most recently from e54ea07 to 4fbbcb1 Compare February 10, 2023 04:46
@jngrad
Remove particle_data.cpp
82cda68 
Rewrite the EspressoSystemStandAlone class and all particle property
setters, getters and events without MpiCallbacks. Run unit tests in
parallel.
@jngrad jngrad force-pushed the parallel-monte-carlo branch from 4fbbcb1 to 82cda68 Compare February 10, 2023 05:48
@jngrad jngrad marked this pull request as ready for review February 10, 2023 17:43
pm-blanco
pm-blanco reviewed Feb 13, 2023
View reviewed changes
Copy link
Contributor

@pm-blanco pm-blanco left a comment

Choose a reason for hiding this comment

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

Thank you again @jngrad for all your hard work on this PR. I think that it looks good to me, except some specific comments that I leave here. Since this PR comes with quite some changes in the code, I tested it for the case of a oligopeptide for which we have published reference data (See fig. attached). The results look good to me within the statistics (the simulations are rather short), so on my side the PR can be merged after addressing my other comments.
test_peptide_short

src/python/espressomd/reaction_methods.py Show resolved Hide resolved
src/python/espressomd/reaction_methods.py Outdated Show resolved Hide resolved
src/core/reaction_methods/ReactionAlgorithm.hpp Outdated Show resolved Hide resolved
src/core/reaction_methods/ReactionAlgorithm.cpp Outdated Show resolved Hide resolved
@jngrad @pm-blanco
core: Give MC functions more descriptive names
4cfc4ce 
Co-authored-by: Pablo Miguel Blanco Andrés <[email protected]>
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

pm-blanco

This comment was marked as outdated.

Sign in to view

@jngrad jngrad added this to the Espresso 4.3.0 milestone Feb 13, 2023
@jngrad jngrad requested a review from reinaual February 13, 2023 18:30
@jngrad @pm-blanco
python: Rename MC method reaction argument
881d214 
For consistency with the integrator.run() method.

Co-authored-by: Pablo Miguel Blanco Andrés <[email protected]>
@jngrad jngrad force-pushed the parallel-monte-carlo branch from d51bc91 to 881d214 Compare February 13, 2023 23:17
reinaual
reinaual previously approved these changes Feb 22, 2023
View reviewed changes
src/python/espressomd/reaction_methods.py Outdated
Comment on lines 508 to 515
for reaction in self.reactions:
reaction = {"reactant_coefficients": reaction.reactant_coefficients,
"reactant_types": reaction.reactant_types,
"product_types": reaction.product_types,
"product_coefficients": reaction.product_coefficients,
"reactant_types": reaction.reactant_types,
"gamma": reaction.gamma}
reactions_list.append(reaction)
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally don't like the name-overriding of variables like here

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch. It's quite dangerous. Not to mention the W0109 pylint violation (key "reactant_types" appears twice) and the amount of copy-pasting that could be elided with a getattr in a list comprehension.

@jngrad
python: Add pylint check W0109 (duplicate-key)
a716f36 
Duplicated dict keys will silently override one another.
@jngrad jngrad requested a review from reinaual February 23, 2023 17:09
@jngrad jngrad added the automerge Merge with kodiak label Feb 24, 2023
@kodiakhq kodiakhq bot merged commit dc9b2ac into espressomd:python Feb 24, 2023
@jngrad jngrad deleted the parallel-monte-carlo branch March 21, 2023 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pm-blanco pm-blanco pm-blanco approved these changes

@reinaual reinaual reinaual approved these changes

@davidbbeyer davidbbeyer Awaiting requested review from davidbbeyer

Assignees
No one assigned
Labels
ApiChange automerge Merge with kodiak Core Improvement Python
Projects
None yet
Milestone
ESPResSo 4.3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@jngrad @reinaual @pm-blanco