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

Rewrite long-range actors #4506

Merged
merged 1 commit into from
May 31, 2022
Merged

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented May 3, 2022

Fixes #4454

Description of changes:

  • apply the SOLID principles
    • split electrostatics from magnetostatics
    • factor out code duplication
    • extract P3M/DP3M tuning code
    • hide ELC details in private class methods (the energy/force correction logic used to be exposed in coulomb.cpp)
    • hide Coulomb implementation details by no longer indirectly including coulomb.hpp in 30 unrelated files in the core
  • write clear pre-conditions as event hooks
    • these events are triggered upon box size change, periodicity change, node grid change and cell structure change
    • these preconditions always run once before the integration loop starts
    • write test cases to check for these events
    • bugfix: charge neutrality checks now run once before the integration loop starts, instead of once at actor activation
  • encapsulate the state of long-range methods in classes managed by the script interface
    • bugfix: the cached DipolarP3M energy correction is now invalidated when the box length changes
    • remove global variables for CPU methods (partial fix for Remove global variables #2628) and introduce one global variable for each type of long-range actor: dipolar solver, Coulomb solver, Coulomb extension
    • convert Cython files for long-range actors to Python files using the ScriptInterface framework
    • the list of active long-range actors can no longer end up in an invalid state (partial fix for Robust exception mechanism in core setters #4219):
      • updating an active actor with an invalid parameter now automatically restores the original parameter
      • inserting an invalid long-range actor in the list of actors now automatically removes the actor from the list of active actors, even when the exception happens during tuning
  • API changes:
    • ELC and DLC are no longer extensions, instead they take another actor as argument
    • P3MGPU is now able to execute the CPU energy and pressure kernels using parameters tuned for the GPU force kernel
    • P3MGPU is now supported by ELC
    • long-range actor parameters are now immutable, since changing one parameter (e.g. the prefactor) would usually invalidate the tuned parameters
    • the charge neutrality check sensitivity can now be modified via actor.charge_neutrality_tolerance; this is mostly relevant to actors coupled to ICC, whose induced charges can have value spanning several orders of magnitude
  • under-the-hood changes:
    • CPU-based long-range actors no longer rely on the MpiCallbacks framework
    • the subtracted Coulomb bonded interaction and pair criteria no longer indirectly include coulomb.hpp

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@jngrad jngrad force-pushed the rewrite_actors branch 2 times, most recently from 786d0c2 to 9ea50b2 Compare May 4, 2022 00:59
@jngrad jngrad requested a review from reinaual May 4, 2022 10:56
@jngrad jngrad force-pushed the rewrite_actors branch from bdea4ba to 860563c Compare May 7, 2022 15:44
Comment on lines +53 to +59
if isinstance(actor, script_interface.ScriptInterfaceHelper):
actor._activate()

self.active_actors.append(actor)

if not isinstance(actor, script_interface.ScriptInterfaceHelper):
actor._activate()
Copy link
Contributor

Choose a reason for hiding this comment

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

where does it come from, that for ScriptInterfaceHelper-objects we need to call _activate() before appending it to the list?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the intended workflow:

  • _activate() calls the C++ actor registration code that adds the actor in the global optional variant
  • the now active actor in the core runs the sanity checks and tuning algorithm (if the method supports tuning)
  • if checks or tuning fail, a C++ exception is thrown, which is handled in the registration code
    • the actor is safely removed from the core global
    • the exception is re-thrown
    • on MPI rank 0, the C++ exception becomes a Python exception
    • line 54 in actors.py throws
    • the python object is never added to the list of active actors

Since removing an actor from the actor list will trigger sanity checks in the core, we cannot remove a python actor if it is not also in the core. We also cannot keep an invalid actor in the core. LB and EK actors still rely on the old workflow.

Comment on lines +84 to +87
tune(charges, positions);
auto const size = static_cast<int>(n_part);
handle_error(fcs_run(m_handle, size, positions.data(), charges.data(),
fields.data(), potentials.data()));
Copy link
Member Author

Choose a reason for hiding this comment

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

Does anyone remember why in the original implementation, we need to re-tune ScaFaCoS before each integration step? Maybe it has to do with side effects when the node_grid or number of particles changes, or when we reload from a checkpoint (tuned parameters cannot be checkpointed). Commenting out line 84 doesn't affect the testsuite, but the edge case it's supposedly guarding against is not documented in the original implementation.

Copy link
Contributor

@reinaual reinaual left a comment

Choose a reason for hiding this comment

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

LGTM

Create more fine-grained events for long-range actors to react to
changes in the cell structure and box geometry. Convert Cython code
to Python code with the ScriptInterface framework and remove global
variables of CPU methods. Fix regressions in the energy and pressure
analysis module. Split electrostatic and magnetostatic methods.
@kodiakhq kodiakhq bot merged commit 58cfc20 into espressomd:python May 31, 2022
@jngrad jngrad deleted the rewrite_actors branch May 31, 2022 15:06
kodiakhq bot added a commit that referenced this pull request Dec 3, 2024
Description of changes:
- Update documentation to reflect that P3MGPU is in fact able to execute the CPU energy and pressure kernels using parameters tuned for the GPU force kernel since #4506.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Long-range actors should be script interface objects P3M GPU has no energy calculation.
2 participants