Skip to content

Conversation

giacomofiorin
Copy link
Member

Fixes #819

@giacomofiorin giacomofiorin added the bugfix To be used only in PRs label Jul 10, 2025
@giacomofiorin
Copy link
Member Author

Still working on this (including test, documentation update).

When finished, I want to use it as a test for a CI job that cherry-picks bugfixes into release branches (e.g. namd-3.0, gromacs-2025).

Comment on lines +581 to +582
// TODO: Is it better to compute the energy from all historic hills
// when keepHills is on?
Copy link
Member Author

Choose a reason for hiding this comment

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

@HanatoK I didn't understand this comment before, and forgot to ask you about it

Copy link
Member

Choose a reason for hiding this comment

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

As I understand, without keepHills, the code only accumulates the hills that are off-grid. However, in theory, those Gaussian hills that have been projected onto the histogram are truncated, which means they do not affect the bias energy of an off-grid position.

With keepHills I think it would be better to compute the bias energy from all hills, no matter whether they are in the histogram.

Copy link
Member

@HanatoK HanatoK left a comment

Choose a reason for hiding this comment

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

In my opinion, the logic of computing bias energy should be the same as colvarbias_meta::calc_energy without adding the new hills:

  bool index_ok = false;
  std::vector<int> curr_bin;

  if (use_grids) {

    curr_bin = values ?
      hills_energy->get_colvars_index(*values) :
      hills_energy->get_colvars_index();

    index_ok = hills_energy->index_ok(curr_bin);

  }

  if ( index_ok ) {
    // index is within the grid: get the energy from there
    for (ir = 0; ir < replicas.size(); ir++) {

      bias_energy += replicas[ir]->hills_energy->value(curr_bin);
      if (cvm::debug()) {
        cvm::log("Metadynamics bias \""+this->name+"\""+
                 ((comm != single_replica) ? ", replica \""+replica_id+"\"" : "")+
                 ": current coordinates on the grid: "+
                 cvm::to_str(curr_bin)+".\n");
        cvm::log("Grid energy = "+cvm::to_str(bias_energy)+".\n");
      }
    }
  } else {
    // off the grid: compute analytically only the hills at the grid's edges
    for (ir = 0; ir < replicas.size(); ir++) {
      calc_hills(replicas[ir]->hills_off_grid.begin(),
                 replicas[ir]->hills_off_grid.end(),
                 bias_energy,
                 values);
    }
  }

Comment on lines +571 to +572
std::vector<int> curr_bin = hills_energy->get_colvars_index();
const bool index_ok = hills_energy->index_ok(curr_bin);
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to move the checking out of the for loop, just like colvarbias_meta::calc_energy?

}
// cvm::log("WARNING: computing bias factor for off-grid hills. Hills energy: " + cvm::to_str(hills_energy_sum_here) + "\n");
} else {
calc_hills(replicas[ir]->hills.begin(), replicas[ir]->hills.end(), hills_energy_sum_here,
Copy link
Member

Choose a reason for hiding this comment

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

Now I feel the old code summing the hills starting from new_hills_begin is incorrect, and this fixes it. Is that right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix To be used only in PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Combine Gaussians for multiple-walkers metadynamics before applying well-tempered scaling
2 participants