Rewrite LB/EK interface #4773
Rewrite LB/EK interface #4773
Conversation
Use lambda captures by reference. Use consistent function names. Remove unused header includes. Reduce usage of the actors list.
The waLBerla module now only uses Boost in the unit tests.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
* remove the actor list * enforce SOLID principles * add fine-grained event hooks
jngrad
force-pushed
the
long_range_actors
branch
from
e0c1f27 to
3c7a5d3
Compare
jngrad
added
Core
Improvement
ApiChange
waLBerla
| # note: several members can only be instantiated once | ||
| if has_features("WALBERLA"): | ||
| if self._lb is not None: | ||
| lb, self._lb = self._lb, None |
There was a problem hiding this comment.
is there a reason why this is set in 2 lines and not just self.lb, self._lb = self._lb, None?
There was a problem hiding this comment.
Unfortunately yes, it has to be written in two lines because of Python's evaluation order.
In more detail: self.lb is a callback method that internally assigns a value to self._lb. We want to back up the self._lb object from the checkpoint file in a temporary variable lb, then assign None to self._lb. Then the expression self.lb = lb can safely initialize self._lb under the hood.
I couldn't find an better syntax due to how pickling works in Python. Once we're done moving all global variables to the core System class, all of this unsustainable Python code will disappear.
src/core/integrate.cpp
Outdated
| } | ||
| lb_lbcoupling_propagate(); | ||
| } else if (lb_active) { | ||
| auto const lb_steps_per_md_step = LB::get_steps_per_md_step(time_step); | ||
| auto &lb = system.lb; | ||
| auto const lb_steps_per_md_step = lb.get_steps_per_md_step(time_step); |
There was a problem hiding this comment.
I find this naming of the variables and functions misleading, because with tau >= dt (which is forced) there are multiple md steps in between 2 lb/ek steps. better would be md_steps_per_lb_step
There was a problem hiding this comment.
Good catch! Not only that, but the assertion was also incorrect, because the EK propagation counter was not synchronized with the LB propagation counter. Let me fix that.
There was a problem hiding this comment.
By the way, we should really think about writing a test for tau > dt with LB + EK active.
src/core/ek/Solver.hpp
Outdated
| */ | ||
| double get_tau() const; | ||
|
|
||
| auto get_steps_per_md_step(double time_step) const { |
There was a problem hiding this comment.
misleading naming see integrate.cpp
src/core/lb/Solver.hpp
Outdated
| void add_force_density(Utils::Vector3d const &pos, | ||
| Utils::Vector3d const &force_density); | ||
|
|
||
| auto get_steps_per_md_step(double time_step) const { |
There was a problem hiding this comment.
misleading naming see integrate.cpp
Description of changes:
espressomd.Systemclass now has a new memberlbespressomd.Systemclass memberekreactionsis now thereactionattribute ofsystem.ekcontainerespressomd.Systemclass no longer has anactorsmemberBoost::unit_test_frameworksrc/grid_based_algorithmswas split intosrc/core/lbandsrc/core/eklattice_switch,ek_container,ek_reactions,lb_walberla_instance,lb_walberla_params_instance) were removed; these objects are now members of theSystemclass(partial fix for Remove global variables #2628)