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

Add checks for cell system compatibility #4428

Closed
RudolfWeeber opened this issue Jan 24, 2022 · 2 comments
Closed

Add checks for cell system compatibility #4428

RudolfWeeber opened this issue Jan 24, 2022 · 2 comments

Comments

@RudolfWeeber
Copy link
Contributor

Goal: Algorithms that rely on specific cell structures should check for them either at the point where the method is called or (if that's impractical) in a sanity check on integration start
(In preparation for #3662)

Steps:

  • In cellStructure.hpp make the type an enum class
  • Move the definition of this enum calss to a separate file CellStructureType.hpp. This let's you use it elsehere without giving access to the full cell structure header.
  • Add a check for the domain decompositoin type to the following places (list may be incompletet). Use Espresso's RuntimeErrorMSG() mechanism where possible.
  • Avoid giving various places in Espresso access to the cell_structure, if they do not need it. Preferrably pass in the cell structure type as an argument.

Relevant places

  • CPU P3M, ELC, ICC (see force_calc() in forces.cpp)
  • Dipolar P3m and DLC
  • The Scafacos electrostatics and magnetostatics solvers
  • LB CPU (propagate and particle coupling. Dispatched from the main integraiotn loop (integrate.cpp) and fore_calc() (forces.cpp), respectively)
  • The external potential featues (?)
  • The Scafacos electrostatics and magnetostatics solvers* Anything else that uses regular grids
    Err on the side of caution.

Places that proabbly don't care about the decompositoin:

  • cluster analysis
  • collision detection
  • reaction ensemble
  • all GPU methods
  • dipolar direct summation,
  • Debye-Hueckel

Unclear:

  • MMM1D
@pkreissl
Copy link
Contributor

pkreissl commented Feb 4, 2022

I will work on this one now.

kodiakhq bot added a commit that referenced this issue Feb 21, 2022
Partial fix for #4428

Description of changes:
- `CellStructureType` is now an `enum class`.
- Removed access to the cell system in several files by storing the information about the current `CellStructureType` in the global variable `LocalBox<double> local_geo`.
kodiakhq bot added a commit that referenced this issue May 31, 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
      - partial fix for #4428
      - setting a `node_grid` incompatible with a long-range actor is no longer possible (the old `node_grid` is restored)
   - 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 #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
     - long-range actors and their python object now have the same lifetime, regardless of whether the actor is active or not (partial fix for #4220)
  - the list of active long-range actors can no longer end up in an invalid state (partial fix for #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
      - closes #239
      - P3M-based reaction scripts run with 2x speed-up when reaction steps take place every 1000 integration steps
   - `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`
@jngrad
Copy link
Member

jngrad commented Aug 4, 2022

All magnetostatic and electrostatic methods now implement such checks, and they are triggered when adding such an actor, or when changing the cell structure while an actor is active.

@jngrad jngrad closed this as completed Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants