Core objects should have the life expectancy of the corresponding Python objects #4220
Labels
Comments
kodiakhq bot
added a commit
that referenced
this issue
Apr 30, 2021
Description of changes: * create a new DLC struct every time a new DLC actor is set up (#4220) * use standard exception mechanism in the DLC /DAWAANR/MDDS core setters (#4219) and test it * non-P3M methods no longer crash the system with `std::abort()` (via `errexit()`) when something unexpected happens * reduce code duplication in DLC by disentangling DLC from P3M * DLC now depends on feature `DIPOLES` instead of `DP3M` (aka `DIPOLES + FFTW`) * ICC now depends on feature `ELECTROSTATICS` instead of `P3M` (aka `ELECTROSTATICS + FFTW`)
|
In the long term, these core objects need to be managed by the ScriptInterface instead of being stored as global variables. |
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`
|
This recommendation has now been implemented in almost all features. The last remaining ones (collision detection, integrators, thermostats) have dedicated tickets. Closing. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Many python objects (e.g. actors and integrators) have a constructor that call a C++ setter that recycles the old core object, i.e. the core setter copies the new parameters passed as arguments into the corresponding fields of an already-existing global struct object, which is default-constructed when the ESPResSo system is instantiated and destroyed when the python session terminates. This approach is error-prone, as it's easy to forget to update one field of the struct, in which case the value that existed before the setter was called will remain in the object (the compiler or IDE cannot issue a warning). This also makes it impossible to exit gracefully from an object setter upon error, e.g. if a combination of parameters is incompatible, since the old state of the object has already been partially or fully overwritten. For an example, see #4211 where the histogram dimensions could be resized to negative values in
LBProfileObservable.hpp(e.g.n_samples_x< 0), in which case the negative value was cast to a large unsigned integer.This can be remediated by instantiating a new object in the setter and running the sanity checks on that new object. If the checks fail, the setter throws and the old state remains untouched. If the checks pass, the old state is replaced by the new one. This approach was implemented for the NpT and Steepest Descent integrators (8977e50) and the RF and DH electrostastic methods (383da42).
The text was updated successfully, but these errors were encountered: