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

Fix sum_hills overflows when doing projections #823

Merged
merged 1 commit into from May 12, 2022

Conversation

GiovanniBussi
Copy link
Member

Description

I am trying to resolve a problem of sum_hills with overflow in exponential when projecting a grid to lower dimension.

I cc people that might be familiar with that part of the code: @carlocamilloni @gtribello @valsson. I noticed that Omar has a copy of this code in ves module, maybe he wants to update it as well.

Changes I did:

  • I made the BiasWeight class store a variable (shift) which is the maximum beta*v seen seen so far
  • at the end of the projection, I add back the shift to the logarithm and reset the shift.
  • I merged two loops here. This is crucial since in this manner the logarithm is taken for one point at a time (and not for all the grid points at the end, as it was before).

As you can see in the regtest, the aim is to be able to integrate a free energy with very large values (note that kBT is very low (0.01)). With the current master branch, the resulting fes.dat would look like this:

#! FIELDS d1 projection
#! SET min_d1 0.726691
#! SET max_d1 1.4841
#! SET nbins_d1  23
#! SET periodic_d1 false
    0.7266910   -0.3044524
    0.7611187   -0.9426032
    0.7955464   -3.8206172
    0.8299740  -11.1560780
    0.8644017  -26.8774481
    0.8988294  -56.3034760
    0.9332571         -inf
    0.9676848         -inf
    1.0021125         -inf
    1.0365401         -inf
    1.0709678         -inf
    1.1053955         -inf
    1.1398232         -inf
    1.1742509         -inf
    1.2086785         -inf
    1.2431062         -inf
    1.2775339         -inf
    1.3119616  -43.4845150
    1.3463893  -19.9096343
    1.3808170   -7.8582589
    1.4152446   -2.4672329
    1.4496723   -0.5443036
    1.4841000   -0.3044522

With the fix, it looks like this

#! FIELDS d1 projection
#! SET min_d1 0.726691
#! SET max_d1 1.4841
#! SET nbins_d1  23
#! SET periodic_d1 false
    0.7266910   -0.3044524
    0.7611187   -0.9426032
    0.7955464   -3.8206172
    0.8299740  -11.1560780
    0.8644017  -26.8774481
    0.8988294  -56.3034760
    0.9332571 -104.1750364
    0.9676848 -171.1615119
    1.0021125 -250.2626861
    1.0365401 -325.9693625
    1.0709678 -378.4433089
    1.1053955 -391.7742547
    1.1398232 -361.7430008
    1.1742509 -297.9659459
    1.2086785 -218.9493543
    1.2431062 -143.4879780
    1.2775339  -83.7886778
    1.3119616  -43.4845150
    1.3463893  -19.9096343
    1.3808170   -7.8582589
    1.4152446   -2.4672329
    1.4496723   -0.5443036
    1.4841000   -0.3044522

All current regtests are not affected.

I think this is a bug fix, so I would merge it to v2.8 and then even check if I can backport to v2.7.

Target release

I would like my code to appear in release v2.8 (possibly v2.7)

@gtribello
Copy link
Member

This looks cool. :-)

GiovanniBussi added a commit that referenced this pull request May 12, 2022
Backported from #823 (needed fix for non stretched Gaussians)

# Conflicts:
#	src/tools/Grid.cpp
@GiovanniBussi GiovanniBussi merged commit 1d45782 into master May 12, 2022
@carlocamilloni carlocamilloni deleted the v2.8-fix-sum-hills-overflow branch November 8, 2022 22:44
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.

None yet

3 participants