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

Refactor bonded interactions storage #4350

Merged
merged 4 commits into from
Oct 26, 2021

Conversation

biermanncarl
Copy link
Contributor

@biermanncarl biermanncarl commented Sep 17, 2021

Fixes #4225
Fixes #4377
Fixes #4169

Description of changes:

  • Bonded interactions are now stored as ScriptInterface objects, are immutable and can be removed
  • It is no longer possible to overwrite a bond object by a different type of bond (avoids segmentation faults)

@biermanncarl biermanncarl force-pushed the fix-4225-carl branch 2 times, most recently from 2a3f144 to 5ebc1aa Compare September 17, 2021 12:27
@biermanncarl biermanncarl force-pushed the fix-4225-carl branch 2 times, most recently from 45cde8b to 23cda43 Compare October 7, 2021 14:31
@biermanncarl biermanncarl changed the title WIP: Refactor bonded interactions storage Refactor bonded interactions storage Oct 18, 2021
@jngrad jngrad self-requested a review October 21, 2021 18:46
jngrad and others added 2 commits October 22, 2021 13:01
Bonded interactions are now instantiated in the script interface.
The core accesses these interactions via shared pointers.
MPI communication is handled by the script interface.

Co-authored-by: Jean-Noël Grad <jgrad@icp.uni-stuttgart.de>
jngrad
jngrad previously approved these changes Oct 22, 2021
Copy link
Member

@jngrad jngrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! From my side, this is ready for merge.

@biermanncarl
Copy link
Contributor Author

Also fixes #4169 ?

@RudolfWeeber
Copy link
Contributor

Thank you, Carl. This is a big step forward.

I think we can merge this now, also to unblock related work.

However, some improvements should still be considered:

  • It looks like the BondedInteraction script interface class still knows about the numerical id (i,e, key) of the bond in the BondedInteractions storage. That should be avoided. The individual bonded interaction should not (need to) know, how it is stored.
  • At least the valid_keys() Python function can probably be autogenerated from the parameter list of the script object.

Deduce BondedInteraction.valid_keys and BondedInteraction.required_keys
from the script interface. Properly serialize IBM bonds. Refactor
script interface __richcmp__ method. Remove throw statements in
script interface callbacks.
@jngrad
Copy link
Member

jngrad commented Oct 25, 2021

At least the valid_keys() Python function can probably be autogenerated from the parameter list of the script object.

Good point! The valid_keys() and required_keys() methods are now deduced from the script object.

It looks like the BondedInteraction script interface class still knows about the numerical id (i,e, key) of the bond in the BondedInteractions storage. That should be avoided. The individual bonded interaction should not (need to) know, how it is stored.

This has to remain so for now, unfortunately. The BondedInteractions container serializes the bond objects by storing the hashmap key (aka bond id) and the bond object parameter dict in the pickle file. During deserialization, a new bond is created from the parameter dict and it is stored in the hashmap with the same key. So in principle we don't need to store the key in the bond object itself, however the bond objects are also stored as a copy in the ParticleHandle objects, and the only way to make the particle bond lists agree with the system bond list after deserialization is to store a unique identifier in the bond objects, e.g. the bond id.

This is not a good design, because each bond is serialized and deserialized multiple times. There is also the issue of the IBM bonds that are hiding the particles ids, and therefore cannot be checkpointed (this is already an issue in the current python branch). I tried to work around that by re-implementing the script interface hashmap in python (4f509f1), but this adds more complexity and doesn't work anyway, because IBM bonds as a side effect do a particle fetch in the constructor, but there is no guarantee that all particles in an IBM bond have already been loaded by the time the first particle in the bond is being loaded.

We can probably revisit this design later, for example in the planned project to store bonds symmetrically on particles.

@jngrad jngrad added the automerge Merge with kodiak label Oct 26, 2021
@kodiakhq kodiakhq bot merged commit de00324 into espressomd:python Oct 26, 2021
@biermanncarl biermanncarl deleted the fix-4225-carl branch October 28, 2021 09:54
kodiakhq bot added a commit that referenced this pull request Nov 23, 2021
Description of changes:
- allow `@property`-setters in `ScriptInterfaceHelper` classes instead of silently skipping them
- unit test the `ScriptInterfaceHelper` class in python
- re-enable benchmark tests
- fix regressions introduced by #4350
- store bond objects in the ObjectMap (see #4391 (comment))
- stop using the core global variable `this_node` in the script interface
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants