Project

General

Profile

Bug #2192

grompp should read floats (e.g charge) from data files to double, to avoid accumulating round-off error

Added by Mark Abraham about 2 years ago. Updated 7 months ago.

Status:
Accepted
Priority:
Low
Assignee:
Category:
preprocessing (pdb2gmx,grompp)
Target version:
Affected version - extra info:
Affected version:
Difficulty:
uncategorized
Close

Description

On the 3.3M lignocellulose benchmark, gmx grompp warns that total charge is -0.000267. In double precision, it does not warn. Each moleculetype in the topology claims a qtot of zero, so probably this is an accumulation issue.

More generally, we should use double anywhere that we haven't measured that performance is worth improving via running in single, just like every other HPC application.

Associated revisions

Revision 635eb138 (diff)
Added by Berk Hess about 2 years ago

Avoid grompp charge warning with rounding

Even though the grompp total charge check uses double for summation,
there are rounding errors for each charge when charges are stored
in single precision. Now the charge check rounds the net charge of
molecules to integer when the difference is less than the maximum
possible sum of charge rounding errors.

Fixes #2192.

Change-Id: I4e24620ed4ff0901b297db4689e75f0befd23944

History

#1 Updated by Mark Abraham about 2 years ago

  • Description updated (diff)

#2 Updated by Berk Hess about 2 years ago

  • Status changed from New to Accepted
  • Assignee set to Berk Hess

All charge summation in grompp (and even in mdrun for PME corrections) is done in double precision. So I don't see directly why things differ here between a single and double build. Maybe reading and storing each charge in single precision leads to a difference in the last significant bit of some charges which is visible in the double.
If that is the case, the check in sum_q in topio.cpp should not check against 1e-6, but against something like 0.5*GMX_REAL_MIN*atoms->nr.

#3 Updated by Gerrit Code Review Bot about 2 years ago

Gerrit received a related patchset '1' for Issue #2192.
Uploader: Berk Hess ()
Change-Id: gromacs~master~I4e24620ed4ff0901b297db4689e75f0befd23944
Gerrit URL: https://gerrit.gromacs.org/6687

#4 Updated by Berk Hess about 2 years ago

  • Tracker changed from Feature to Bug
  • Status changed from Accepted to Fix uploaded
  • Target version set to 2018
  • Affected version set to git master

I would say this is a bug, since with PME grompp will generate a warning (not in v2016) and -maxwarn 1 is needed.
I pushed up a change that increases the tolerance for rounding for large molecules. Now we tolerate larger errors for large molecules. The only way to avoid that is reading and storing the charges in double precision in grompp.

#5 Updated by Berk Hess about 2 years ago

  • Subject changed from grompp should accumulate total charge in double precision to grompp charge precision leads to incorrect total charge warning

#6 Updated by Erik Lindahl about 2 years ago

This is a really old issue. I remember looking into it, and the problem is likely that the individual charges themselves with values like 0.3 cannot be represented exactly in floating-point data (and obviously the error is much larger in single), and then it doesn't help that we perform the summation in double.

#7 Updated by Berk Hess about 2 years ago

  • Status changed from Fix uploaded to Resolved

#8 Updated by Mark Abraham almost 2 years ago

  • Subject changed from grompp charge precision leads to incorrect total charge warning to grompp should read floats (e.g charge) from data files to double, to avoid accumulating round-off error
  • Status changed from Resolved to Accepted
  • Target version changed from 2018 to 2019

Retargeted the remaining part of the task to 2018

#9 Updated by Paul Bauer 8 months ago

The reading is already done in double (see toppush.cpp line 241 and 324). Precision is lost when assigned to the underlying data structure in line 484. So fixing the rest would mean changing this in the t_atoms.

#10 Updated by Paul Bauer 7 months ago

  • Target version changed from 2019 to 2020

Changing the representation in the TPR would lead to a number of different issues, so this likely won't happen for 2019.

Also available in: Atom PDF