Refactor Observable_stat framework #3712
Conversation
Use size_t consistently and return coulomb/dipolar sizes by value instead of using output parameters.
Move the reallocation logic into the observable struct and narrow function signature.
Moving update_pressure() to pressure.cpp significantly narrows down the number of exported pressure functions.
Rewrite mpi_gather_stats() with an enum class for the job type and document the output parameters.
Reduce code duplication and hide some implementation details.
Having two flags helps in reducing code duplication.
Fix regression from 8bcb6d4.
Exposing raw pointers to the underlying C-array of the std::vector is prone to buffer overflows, which will happen if the contributions from coulomb and dipole are written after the contributions from virtual sites and external fields when no coulomb and dipole methods are active. With Spans, a view is returned whose operator[] guards against out-of-bounds access via assertions.
Make the observables fully self-contained. Remove the manual reallocation management logic. Adapt the Coulomb/Dipole code accordingly. Keep track of created observables instead of manually curating a list of global variables in statistics.cpp.
Remove dependency on statistics.hpp from most of the core. Reduce the visibility of Observable_stat globals.
Remove the runtime error message side-effect. The inlined versions are trivial; there is no need for callbacks in the Observable_stat class.
Resize and zero-out local (aka MPI rank-specific) obsstats. Resize but don't zero-out total (aka on master node) obsstats, since the reduction will overwrite old values. Move the `is_initialized` and `v_comp` flags to the wrapper classes and only invalidate on system change. There is no need to reallocate on system change, since invalidation already implies reallocation, thus the reallocation callbacks can be removed. Reduce code duplication in pressure.cpp.
|
@fweik @RudolfWeeber the old implementation with raw pointers gave me quite some trouble. The coulomb/dipolar contributions were overflowing a zero into the virtual site and external field contributions when no coulomb/dipolar method was active, because some places in the coulomb/dipolar calculation code forgot to guard against This PR achieved most of the work needed to convert
If moving to genuine |
Codecov Report
@@ Coverage Diff @@
## python #3712 +/- ##
======================================
- Coverage 88% 88% -1%
======================================
Files 543 545 +2
Lines 24776 24718 -58
======================================
- Hits 21835 21776 -59
- Misses 2941 2942 +1
Continue to review full report at Codecov.
|
Also remove unused header files and move CUDA helper function back into the corresponding .cu file.
|
I'll have a look this afternoon. I'm not convinced yet that we'll keep all the functionality, we'll see... |
|
@jngrad Do I understand it correctly that the globals are only needed to avoid recalculation of the observables? |
|
offline discussion: By the looks of it, these globals are write-only from the point of view of the core. They are read only from the ScriptInterface and the StressTensor observable. There is no need to cache these values. We could just split up the different contributions into separate observables, and remove all the MPI callbacks triggered by system changes. This can be done in a separate PR. |
The value stored in the local (aka MPI rank-specific) obsstat could not be read, since it was not followed by a reduction on the master node.
The intra- and inter-molecular parts of the non-bonded contributions to the energy are not implemented in the core.
Description of changes:
Observable_statstructs to self-contained classesObservable_statobjectsUtils::SpanObservable_statlist ininvalidate_obs()by a self-updating callback liststatistics.hppincludes (it's now only visible to 9 source files)energy()now returns the lower triangle of the non-bonded IAs, to be consistent withpressure()andstress_tensor()