Project

General

Profile

Task #1345

Charmm - CMAP weirdness in grompp

Added by David van der Spoel over 7 years ago. Updated over 6 years ago.

Status:
Closed
Priority:
Low
Assignee:
Erik Lindahl
Category:
preprocessing (pdb2gmx,grompp)
Target version:
5.0
Difficulty:
uncategorized
Close

Description

While debugging http://redmine.gromacs.org/issues/1343 I stumbled over the following code in grompp (from 4.5 and still present):

plist, the force field parameters etc. is initiated as an array:

snew(plist,F_NRE);
  init_plist(plist);
<snip>
        /* If we are using CMAP, setup the pre-interpolation grid */
        if(plist->ncmap>0)
        {
                init_cmap_grid(&sys->ffparams.cmap_grid, plist->nc, plist->grid_spacing);
                setup_cmap(plist->grid_spacing, plist->nc, plist->cmap,&sys->ffparams.cmap_grid);
        }

In other words, this if statement is checking the first entry in the plist array, which is F_LJ, and hence I assume the if statement always evaluates to false.

It could of course be that the if statement is irrelevant.

Associated revisions

Revision c1547047 (diff)
Added by Erik Lindahl over 6 years ago

Clean up CMAP placement in parameter list

Some of the CMAP variables were always placed into and
read from the first entry of the parameter list. While this
did not result in any errors, this patch now places them
correctly in the F_CMAP position.

Fixes #1345.

Change-Id: Ic6e09e46c352976cfc1f57ff903c082b4ec43df8

History

#1 Updated by David van der Spoel over 6 years ago

  • Assignee changed from David van der Spoel to Erik Lindahl

#2 Updated by Roland Schulz over 6 years ago

  • Status changed from New to Feedback wanted

push_cmaptype uses "bt->ncmap" and bt is plist. Thus plist0 is used for all CMAP access. This is really confusing code but it does seem consistent and not a bug. Do I you agree?

#3 Updated by David van der Spoel over 6 years ago

  • Tracker changed from Bug to Feature
  • Priority changed from Normal to Low
  • Target version set to 5.x

It is not a bug in the sense that the code probably works. In grompp.h we have:

typedef struct {
int nr; /* The number of bonds in this record /
int maxnr; / The amount of elements in the array /
t_param *param; / Array of parameters (dim: nr) */

/* CMAP tmp data, there are probably better places for this /
    int        grid_spacing; / Cmap grid spacing /
    int        nc;           / Number of cmap angles */
real       cmap;  / Temporary storage of the raw cmap grid data /
    int        ncmap;    / Number of allocated elements in cmap grid*/
int        cmap_types;  / Store the five atomtypes followed by a number that identifies the type /
    int         nct;         / Number of allocated elements in cmap_types */

} t_params;

Note the comment by the author, presumably Erik's old student.
Simultaneously, there is F_CMAP entry in the list of energies which I presume is used for storing the energy. Thus plist[F_CMAP] should be used for storing the grompp data as well.

All in all, this is the kind of code that will come back to haunt us, but it not being a bug in practice, I will reduce the severity to low, and change it to a feature request - for cleanup.

#4 Updated by Roland Schulz over 6 years ago

  • Status changed from Feedback wanted to Accepted

#5 Updated by Gerrit Code Review Bot over 6 years ago

Gerrit received a related patchset '1' for Issue #1345.
Uploader: Erik Lindahl ()
Change-Id: Ic6e09e46c352976cfc1f57ff903c082b4ec43df8
Gerrit URL: https://gerrit.gromacs.org/3662

#6 Updated by Erik Lindahl over 6 years ago

  • Status changed from Accepted to Fix uploaded

#7 Updated by Erik Lindahl over 6 years ago

  • Status changed from Fix uploaded to Resolved

#8 Updated by Erik Lindahl over 6 years ago

  • Status changed from Resolved to Closed

#9 Updated by Teemu Murtola over 6 years ago

  • Tracker changed from Feature to Task
  • Target version changed from 5.x to 5.0

Also available in: Atom PDF