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

Ability to define custom grid in ABF biases #710

Closed
wants to merge 11 commits into from
Closed

Ability to define custom grid in ABF biases #710

wants to merge 11 commits into from

Conversation

jhenin
Copy link
Member

@jhenin jhenin commented Aug 30, 2024

This creates a new keyword, gridParameters, which mimics histogramGrid within a histogram bias.
In the process, unify both keywords under the new name and deprecate histogramGrid.

See Issue #476

TODO:

  • tests
  • doc
  • refine doc to make it an independent section

@HanatoK
Copy link
Member

HanatoK commented Aug 30, 2024

It is good to see that the grid parameters can be separated from the colvar{} definitions. Do you think that it would be useful if the CZAR histogram could have a different set of grid parameters?

@jhenin
Copy link
Member Author

jhenin commented Sep 1, 2024

No, I can't think of a use case for having CZAR on a different grid.

@giacomofiorin
Copy link
Member

@jhenin I am reviewing this, and as you know I like the approach (see #476), but I am also remembering now an unwanted consequence of encouraging the users to use custom definitions of the grid as their go-to option.

Metadynamics exhibits artifacts when the system routinely samples states that are too close to the grid boundaries, as you know. https://colvars.github.io/master/colvars-refman-namd.html#sec:colvarbias_meta_boundaries
Neither allowing the simulation to get stuck because of these artifacts or killing performance keeping a large number of hills is desirable. I remember speaking with several frustrated users of the Colvars metadynamics implementation at the time.

The current code and documentation are now encouraging users to just use the mathematical boundaries of the CVC functions (see #310), i.e. not define grid boundaries themselves when default values are available.

If we merge this PR (as I think we will very soon), we would also need to improve the code introduced in #310 and let it check that each user-provided boundary matches the default given by the CVCs. Would you be okay with waiting to contribute to the MD engine distributions until these changes are also merged?

@jhenin
Copy link
Member Author

jhenin commented Nov 12, 2024

This is now superseded by #733

@jhenin jhenin closed this Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants