Project

General

Profile

Bug #1232

Incorrect calculation of TI kinetic energy contribution -- no zeroing

Added by Michael Shirts over 4 years ago. Updated over 4 years ago.

Status:
Closed
Priority:
High
Category:
mdrun
Target version:
Affected version - extra info:
4.6 and 4.6.1
Affected version:
Difficulty:
uncategorized
Close

Description

There is an incorrect accumulation of the thermodynamic integration kinetic energy contribution. This appears to be because the
accumulator term:

dekindl_sum = &ekind->ekin_work[thread][opts->ngtc][0][0];

is never zeroed. It does not show up in some simulations because of another error in handling F_DVDL and F_DKDL that in some situations causes F_DKDL not to be printed.

I am getting a patch ready to post in the next 30 min or so.

Associated revisions

Revision b8440aeb (diff)
Added by Michael Shirts over 4 years ago

Fixing handling of perturbation mass changes.

Fixes redmine #1232

in force.c, sum_dhdl

  • moved F_DKDL to match the order in efpt_names. Not required, but
    harmonizes the code (lack of clarity probably helped cause the
    problems before), has no code effect.
  • no longer treating the F_DKDL term separately from the other
    derivative components. Will be added to F_DVDL if the mass-lambda
    term is not separately specified. Results in a bit of a misnomer
    (F_DVDL becomes the derivative of the entire hamiltonian), but
    makes it much easier to collapse all molecular perturbation terms
    into a single component for output, where it is no longer really
    F_DVDL. I think that's better than always printing out a F_DVDL
    and a F_DKDL for everything where F_DKDL will probably usually
    be zero.

in md_support.c, compute_globals

  • Synchronize the behaviors of the dhdls by writing first to the linear component
    corresponding to the mass, and then later transferring it to F_DKDL

in group.h, struct gmx_ekindata_t * add pointer to per-thread accumulation variable for dekindl

in tgroup.c, sum_ekin

  • For velocity verlet integrators, computes the dekindl correctly as
    the derivatives of the current ekin. Shouldn't really affect the results
    in any significant way, since the average contribution will be the same
    regardless, but this is more consistent.

in tgroup.c, init_ekindata

  • reduce use of numeric constants in allocating memory
  • initialize new ekindata_t member

in update.c, calc_ke_part_normal

  • zero the accumulator for dekindl before using it,
    fixing bug introduced in 7b6508e8

in update.c, in calc_ke_part_normal and calc_ke_part_visc

  • sign error in mass change; if mass B is greater than mass A, then the
    change in free energy is positive, not negative.

Change-Id: I9deaf546bca66d400e0eb2c4015abeeda302dd1d

Revision 3e918e6d (diff)
Added by Michael Shirts over 4 years ago

Fixing handling of perturbation mass changes.

Fixes redmine #1232

in force.c, sum_dhdl

  • moved F_DKDL to match the order in efpt_names. Not required, but
    harmonizes the code (lack of clarity probably helped cause the
    problems before), has no code effect.
  • no longer treating the F_DKDL term separately from the other
    derivative components. Will be added to F_DVDL if the mass-lambda
    term is not separately specified. Results in a bit of a misnomer
    (F_DVDL becomes the derivative of the entire hamiltonian), but
    makes it much easier to collapse all molecular perturbation terms
    into a single component for output, where it is no longer really
    F_DVDL. I think that's better than always printing out a F_DVDL
    and a F_DKDL for everything where F_DKDL will probably usually
    be zero.

in md_support.c, compute_globals

  • Synchronize the behaviors of the dhdls by writing first to the linear component
    corresponding to the mass, and then later transferring it to F_DKDL

in group.h, struct gmx_ekindata_t * add pointer to per-thread accumulation variable for dekindl

in tgroup.c, sum_ekin

  • For velocity verlet integrators, computes the dekindl correctly as
    the derivatives of the current ekin. Shouldn't really affect the results
    in any significant way, since the average contribution will be the same
    regardless, but this is more consistent.

in tgroup.c, init_ekindata

  • reduce use of numeric constants in allocating memory
  • initialize new ekindata_t member

in update.c, calc_ke_part_normal

  • zero the accumulator for dekindl before using it,
    fixing bug introduced in 7b6508e8

in update.c, in calc_ke_part_normal and calc_ke_part_visc

  • sign error in mass change; if mass B is greater than mass A, then the
    change in free energy is positive, not negative.

Change-Id: I9deaf546bca66d400e0eb2c4015abeeda302dd1d

History

#1 Updated by Michael Shirts over 4 years ago

  • Assignee changed from Berk Hess to Michael Shirts

#2 Updated by Michael Shirts over 4 years ago

  • Target version set to 4.6.2

#3 Updated by Michael Shirts over 4 years ago

  • % Done changed from 50 to 90
  • Affected version - extra info set to 4.6 and 4.6.1

OK, posted a fix.

#4 Updated by Mark Abraham over 4 years ago

  • Status changed from New to Accepted

#5 Updated by Mark Abraham over 4 years ago

  • Status changed from Accepted to In Progress

Indeed. Bug was introduced in commit 7b6508e8 (the big squashed Verlet commit).

#6 Updated by Mark Abraham over 4 years ago

  • Status changed from In Progress to Fix uploaded

#7 Updated by Mark Abraham over 4 years ago

  • Status changed from Fix uploaded to Resolved

#8 Updated by Mark Abraham over 4 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF