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

Separation of concerns in Reaction Ensemble #4155

Closed
jngrad opened this issue Mar 11, 2021 · 0 comments · Fixed by #4196
Closed

Separation of concerns in Reaction Ensemble #4155

jngrad opened this issue Mar 11, 2021 · 0 comments · Fixed by #4196
Assignees

Comments

@jngrad
Copy link
Member

jngrad commented Mar 11, 2021

The RE needs to be moved to a submodule of the core and split in several files, one per RE class (along with their dependencies). Right now all classes are mixed together, which makes maintenance more difficult. For example:

  • It is not immediately clear which utility classes are needed by each RE class. It's also unclear if RE classes depend on each other, or if they only depend on the parent class ReactionAlgorithm. Having separate files would help figure that information out by simply reading the #include statements.
  • The public API of RE includes particle_data.hpp and energy.hpp, both of which shouldn't be exposed. They are actually only needed in the implementation of private methods of the CollectiveVariable classes and could easily be moved to the .cpp file (the overhead of not having them inline shouldn't matter much). particle_data.hpp in particular contains prototypes for particle communication and should only be included in .cpp files; in fact, the Particle structs were extracted from particle_data.hpp and stored in Particle.hpp in ESPResSo 4.2 to help us hide the communication logic and remove as many #include <particle_data.hpp> as possible (75ef537, 78bf03d).

The new layout could look like this:

src/core/reaction_ensemble/
                          |- CMakeLists.txt
                          |- utils.hpp
                          |- utils.cpp
                          |- ReactionAlgorithm.hpp
                          |- ReactionAlgorithm.cpp
                          |- ConstantpHEnsemble.hpp
                          |- ConstantpHEnsemble.cpp
                          |- WidomInsertion.hpp
                          |- WidomInsertion.cpp
                          |- ReactionEnsemble.hpp
                          |- ReactionEnsemble.cpp
                          |- WangLandauReactionEnsemble.hpp
                          |- WangLandauReactionEnsemble.cpp

Special files:

  • utils.hpp
  • ReactionAlgorithm.hpp
    • class ReactionAlgorithm
    • class SingleReaction
    • class StoredParticleProperty
  • WangLandauReactionEnsemble.hpp
    • class WangLandauReactionEnsemble
    • class CollectiveVariable
    • class EnergyCollectiveVariable
    • class DegreeOfAssociationCollectiveVariable
@jngrad jngrad self-assigned this Mar 30, 2021
@kodiakhq kodiakhq bot closed this as completed in #4196 Apr 6, 2021
kodiakhq bot added a commit that referenced this issue Apr 6, 2021
Fixes #4155

Description of changes:
- move each class in the ReactionEnsemble into a separate source file
   - helps with separation of concerns and readability
   - Wang-Landau and its collective variables are in the same file due to coupling
- rename the namespace `ReactionEnsemble` to `ReactionMethods`
   - fixes the name collision with class `ReactionEnsemble` (used to be an issue in the class unit test)
   - chose `ReactionMethods` over the other candidates proposed in #3284 since the main class `ReactionAlgorithm` is currently still designed to use the `SingleReaction` class; the discussion in #3284 regarding class names remains open
- move the classes and unit tests to subfolder `src/core/reaction_methods`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant