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

Fix Constraint checkpointing #3105

Merged
merged 9 commits into from
Aug 30, 2019
Merged

Fix Constraint checkpointing #3105

merged 9 commits into from
Aug 30, 2019

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented Aug 26, 2019

Fixes #2943

Enable checkpointing of all Constraint classes which are not based on shapes. This also solves the iterator issue in #2943.

API change: it is now required to instantiate all Constraint classes with named arguments instead of positional arguments.

To build a new Python Constraint object from a core Constraint
object, a Cython PObjectId object is passed as a named argument
"oid" to the Python Constraint __init__() method. All Constraint
classes must provide an __init__(**kwargs) method without any
positional argument for this to work. This fixes the issues with
checkpointing and visualization of constraints not based on shapes.
Python Constraint classes deriving from _Interpolated interface
with core classes which serialize a boost::multi_array<int, 3>
member as a std::vector<int> object, which cannot be deserialized
as an Utils::Vector3i. This fixes the issue with checkpointing of
constraints based on interpolation.
@codecov
Copy link

codecov bot commented Aug 26, 2019

Codecov Report

Merging #3105 into python will increase coverage by <1%.
The diff coverage is 85%.

Impacted file tree graph

@@           Coverage Diff           @@
##           python   #3105    +/-   ##
=======================================
+ Coverage      83%     84%   +<1%     
=======================================
  Files         530     530            
  Lines       26141   26142     +1     
=======================================
+ Hits        21954   21967    +13     
+ Misses       4187    4175    -12
Impacted Files Coverage Δ
src/core/field_coupling/fields/Interpolated.hpp 100% <100%> (ø) ⬆️
src/script_interface/constraints/fields.hpp 91% <100%> (+15%) ⬆️
src/script_interface/get_value.hpp 86% <75%> (-1%) ⬇️
src/core/electrostatics_magnetostatics/p3m.cpp 85% <0%> (-1%) ⬇️
...script_interface/constraints/ExternalPotential.hpp 75% <0%> (+5%) ⬆️
src/script_interface/constraints/ExternalField.hpp 88% <0%> (+5%) ⬆️
src/script_interface/constraints/couplings.hpp 65% <0%> (+8%) ⬆️
src/core/constraints/ExternalPotential.hpp 100% <0%> (+10%) ⬆️
src/core/constraints/ExternalField.hpp 100% <0%> (+10%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2e7c5d...4382a4a. Read the comment docs.

@jngrad
Copy link
Member Author

jngrad commented Aug 26, 2019

@fweik I'm not sure how to interpret the ASAN error stack-use-after-scope here. It's triggered by:

{"_field_data", AutoParameter::read_only, [this_]() {
auto &field_data = this_().field_data();
auto data_ptr =
reinterpret_cast<const double *>(field_data.data());
return std::vector<double>(
data_ptr, data_ptr + codim * field_data.num_elements());
}}};

When adding a printf to show data_ptr[0], it becomes an ASAN heap-use-after-free error. Is the boost::multi_array returned by this_().field_data() holding an array that has been freed by the time we run the lambda in the AutoParameter constructor above? Here is the field_data() function:

storage_type const &field_data() const { return m_global_field; }

This issue didn't show up before because field_params_impl<Interpolated<T, codim>>::params() isn't tested.

@jngrad
Copy link
Member Author

jngrad commented Aug 27, 2019

The two ASAN errors stack-use-after-scope and heap-use-after-free are now gone in the Clang version installed in the docker image after 74ff66e. On the older Clang version installed at the ICP, an ASAN error is still raised but it doesn't show a backtrace nor an error message, only the string "((IsAligned((uptr)ptr, alignment))) != (0)" (0x0, 0x0) (fixed by D44404 in our Docker image). Still not sure why the boost::multi_array would be memcopied somewhere else and the original array freed in the middle of a simulation setup.

@jngrad
Copy link
Member Author

jngrad commented Aug 27, 2019

Nevermind, the cryptic error message fixed by D44404 pops up everywhere when running the core unit tests of the python branch with our outdated Clang compiler. We can safely ignore it.

@jngrad
Copy link
Member Author

jngrad commented Aug 30, 2019

bors r=RudolfWeeber

bors bot added a commit that referenced this pull request Aug 30, 2019
3105: Fix Constraint checkpointing r=RudolfWeeber a=jngrad

Fixes #2943

Enable checkpointing of all `Constraint` classes which are not based on shapes. This also solves the iterator issue in #2943.

API change: it is now required to instantiate all `Constraint` classes with named arguments instead of positional arguments.


3109: Use directed acyclic graphs in gitlab CI r=jngrad a=RudolfWeeber



Co-authored-by: Jean-Noël Grad <jgrad@icp.uni-stuttgart.de>
Co-authored-by: Rudolf Weeber <weeber@icp.uni-stuttgart.de>
@bors
Copy link
Contributor

bors bot commented Aug 30, 2019

Build succeeded

@bors bors bot merged commit 4382a4a into espressomd:python Aug 30, 2019
@jngrad jngrad deleted the fix-2943 branch January 18, 2022 12:10
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.

Some constraints cannot be indexed
2 participants