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.

Sign up for GitHub

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

Jump to bottom

reaction_ensemble module contains several independent classes #3284

Closed
jonaslandsgesell opened this issue Oct 29, 2019 · 1 comment
Closed

reaction_ensemble module contains several independent classes #3284

jonaslandsgesell opened this issue Oct 29, 2019 · 1 comment

Comments

@jonaslandsgesell
Copy link
Member

the reaction_ensemble class now contains several methods, not just the eponymic ReactionEnsemble. Perhaps it would be more appropriate to give a more generic name to the whole class, e.g. reaction_mehods or chemical_reactions.

Please also note that the Reaction ensemble itself hosts a MC method for configurational moves in the canonical ensemble (i.e. acc=min(1,exp(-\beta Delta E)))

Not sure what a good name for the module would be... Maybe something related to Monte-Carlo methods.

Originally posted by @kosovan in #3254
@jonaslandsgesell jonaslandsgesell mentioned this issue Oct 29, 2019
Merged
@kosovan
Copy link
Contributor

kosovan commented Oct 31, 2019

I think that MC methods is a good name for the class, if there are no other MC methods implemented in some other classes, Otherwise, it might lead to even more confusion than now.

@jngrad jngrad mentioned this issue Jun 26, 2020
Closed
@jngrad jngrad mentioned this issue Apr 1, 2021
Merged
kodiakhq bot added a commit that referenced this issue Apr 6, 2021
@kodiakhq
Reorganize Reaction Ensemble (#4196)
f1f6b69 
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
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@kosovan @jonaslandsgesell