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

Let reaction ensemble use observables/accumulators #4156

Closed
RudolfWeeber opened this issue Mar 11, 2021 · 1 comment · Fixed by #4288
Closed

Let reaction ensemble use observables/accumulators #4156

RudolfWeeber opened this issue Mar 11, 2021 · 1 comment · Fixed by #4288

Comments

@RudolfWeeber
Copy link
Contributor

The reaction ensemlbe currently has its own "observables/accumulators", called collective variables.
It should instead use Espresso's observable and accumulator framework.

  1. As a first step, the degree of association collective variable should be implemented as an observable in the sence of the observable framework.
    In the first step, the observable can be stored in the collective variable struct of the RE and be evaluated, whenever the determin_current_state() method of the collective variable in RE is called.
    Same can be done for the potential energy collective var.

  2. In a second step, the minimum and maximum logic of the collective vars can be replaced by storing an accumulator in the collective var struct and have it calculate min/max/average.

  3. In a final step, the Python ui can be modified, so the user can pass in any observable as a collective var.

the first two should be doable in a coding day.

@jngrad
Copy link
Member

jngrad commented Mar 26, 2021

The DegreeOfAssociationCollectiveVariable behaves like an observable, but I'm not sure the EnergyCollectiveVariable does. The EnergyCollectiveVariable depends on the state of the Wang-Landau system. It's also unclear to me whether the minimum and maximum can be calculated from an accumulator, because the minimum and maximum of one collective variable depends on the minimum and maximum of all previously registered collective variables (I had a a surprise when unit testing a second collective variable: 76332ae).

@jngrad jngrad added the Core label May 25, 2021
@kodiakhq kodiakhq bot closed this as completed in #4288 Jul 1, 2021
kodiakhq bot added a commit that referenced this issue Jul 1, 2021
The current Wang-Landau Reaction Ensemble (WLRE) implementation is tightly coupled to the other reaction methods in the C++ core. This accidental complexity slows down the development and improvement of the other reaction methods. Rewriting the WLRE method to improve modularity would be a significant project for the core team. This reaction method is not actively used by the community, and a recent call on the ESPResSo mailing list did not receive any response against the removal of the method:
https://lists.nongnu.org/archive/html/espressomd-users/2021-06/msg00001.html

Partial fix for #4251 (removing WLRE hooks)
Closes #4156

Description of changes:
- Remove the unused WLRE method. It is not actively maintained and not fully tested (86% code coverage).
- Fix undefined behavior in `ReactionAlgorithm` (member `particle_inside_exclusion_radius_touched` was uninitialized).
- Write additional unit tests.
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.

2 participants