Task #1345
Charmm - CMAP weirdness in grompp
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
History
#1 Updated by David van der Spoel over 5 years ago
- Assignee changed from David van der Spoel to Erik Lindahl
#2 Updated by Roland Schulz over 5 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 5 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 5 years ago
- Status changed from Feedback wanted to Accepted
#5 Updated by Gerrit Code Review Bot over 5 years ago
Gerrit received a related patchset '1' for Issue #1345.
Uploader: Erik Lindahl (erik@kth.se)
Change-Id: Ic6e09e46c352976cfc1f57ff903c082b4ec43df8
Gerrit URL: https://gerrit.gromacs.org/3662
#6 Updated by Erik Lindahl over 5 years ago
- Status changed from Accepted to Fix uploaded
#7 Updated by Erik Lindahl over 5 years ago
- Status changed from Fix uploaded to Resolved
#8 Updated by Erik Lindahl over 5 years ago
- Status changed from Resolved to Closed
#9 Updated by Teemu Murtola over 5 years ago
- Tracker changed from Feature to Task
- Target version changed from 5.x to 5.0
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