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 System class #4816

Merged
merged 18 commits into from
Nov 20, 2023
Merged

Refactor System class #4816

merged 18 commits into from
Nov 20, 2023

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented Oct 24, 2023

Description of changes:

  • encapsulate several global variables inside the System class
  • make energy/pressure/force calculation functions members of the System class
  • fully encapsulate event hooks in the System class
  • simplify checkpointing of the System class
  • remove EspressoSystemStandAlone class
  • API changes:
    • espressomd.galilei.GalileiTransform is now a member of the System class, accessible via system.galilei

Remove the non-bonded IA parameters global variable. Replace
upper triangular matrix with lower triangular matrix indexing,
which is more efficient and doesn't require re-ordering stored
objects when resizing the container. Store the actual particle
type in global variable max_seen_particle_type.
Remove partCfg singleton. Rewrite the remaining analysis methods
that ran only on MPI rank 0 to run in parallel, or to stream only
the relevant data structures to MPI rank 0 (instead of the entire
particle data).
Give each member of ScriptInterface::System a handle to the core
System class. Refactor AutoParameters serialization. The System
class can now handle checkpointing of its ObjectHandle members.
@jngrad jngrad force-pushed the system branch 4 times, most recently from f44ac07 to 7a01c10 Compare October 24, 2023 14:59
@jngrad jngrad added this to the ESPResSo 4.3.0 milestone Oct 27, 2023
@jngrad jngrad marked this pull request as ready for review October 27, 2023 19:08
@jngrad jngrad requested review from reinaual and removed request for RudolfWeeber November 7, 2023 12:09
@RudolfWeeber
Copy link
Contributor

CellStructure::set_verlet_skina call of may lead to a situaoitn, where the ParticleDecompositoin needs to be rebuild, or at least the Verlet list becomes invalid. Can we safeguard, that this is not called without hte necessary steps being taken?

@RudolfWeeber
Copy link
Contributor

As I understand it, there is code in src/core/energy.cpp that implements member functions of the System class. Should we move all files that direclty contribute to the system class into a common directory src/core/system or the like?

@RudolfWeeber
Copy link
Contributor

void integrator_sanity_checks() should probably check for time_step <= 0

@RudolfWeeber
Copy link
Contributor

Could you please add some notes as to the purpose of code in leaf.hpp

@RudolfWeeber
Copy link
Contributor

Aside form previous commens, LGTM. This PR is a big step towards getting rid of globals and allowing more than one system in a script. Thanks alot.

Dependency inversion: let CellStructure manage the Verlet skin
and the on_verlet_skin_change() event.
Encapsulation: the core System class now has access to its own
shared_ptr and can bind Leaf objects during construction.
Remove the EspressoSystemStandAlone class, which is now redundant.
@jngrad
Copy link
Member Author

jngrad commented Nov 17, 2023

void integrator_sanity_checks() should probably check for time_step <= 0

Done.

CellStructure::set_verlet_skina call of may lead to a situaoitn, where the ParticleDecompositoin needs to be rebuild, or at least the Verlet list becomes invalid. Can we safeguard, that this is not called without hte necessary steps being taken?

Done. I also took this opportunity to move the Verlet skin management and ghost update logic to CellStructure.

Could you please add some notes as to the purpose of code in leaf.hpp

Done. Can be found in a self-contained .dox file that generates doc/doxygen/html/SystemClassDesign.html. Writing this down in the System class docstring would have forced a rebuild of most of the codebase after each update to the text.

As I understand it, there is code in src/core/energy.cpp that implements member functions of the System class. Should we move all files that direclty contribute to the system class into a common directory src/core/system or the like?

I thought about it, in fact pressure, forces and tuning also implement System methods, but moving them would make the PR diff unreadable, and could introduce merge conflicts with the work of Julian or Thilo. I would prefer to leave these files here for now. It's also quite possible that we later end up merging the associated *_inline.hpp directly in the .cpp files, since most of the inline functions are actually only used there now, and can be made static.

@jngrad jngrad added the automerge Merge with kodiak label Nov 20, 2023
@kodiakhq kodiakhq bot merged commit 38a5338 into espressomd:python Nov 20, 2023
10 checks passed
@jngrad jngrad deleted the system branch November 20, 2023 16:17
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.

None yet

2 participants