Skip to content

Small fix to OpenMM torsion assignment#19

Merged
swails merged 1 commit intoTinkerTools:masterfrom
swails:omm_tors
Sep 9, 2015
Merged

Small fix to OpenMM torsion assignment#19
swails merged 1 commit intoTinkerTools:masterfrom
swails:omm_tors

Conversation

@swails
Copy link
Collaborator

@swails swails commented Aug 21, 2015

Fix OpenMM torsion assignment for amoebabio09 FF (for nucleic acids). The 4-,
5-, and 6-fold torsions were not being added to the OpenMM System, and torsions
with these periodicities were defined for some nucleic acid systems. This commit
makes sure that those torsions are correctly added and adjusts the behavior of
adding torsions such that only torsions with non-zero force constants actually
get added. This reduces the number of "useless" terms in the resulting OpenMM
System and makes debugging easier.

I also added a protective #ifdef __INTEL_COMPILER around the ifort-specific
OpenMP directives so that you don't have to keep changing the source code to
compile with ifort or gfortran.

Fix OpenMM torsion assignment for amoebabio09 FF (for nucleic acids). The 4-,
5-, and 6-fold torsions were not being added to the OpenMM System, and torsions
with these periodicities were defined for some nucleic acid systems. This commit
makes sure that those torsions are correctly added and adjusts the behavior of
adding torsions such that only torsions with non-zero force constants actually
get added.  This reduces the number of "useless" terms in the resulting OpenMM
System and makes debugging easier.

I also added a protective #ifdef __INTEL_COMPILER around the ifort-specific
OpenMP directives so that you don't have to keep changing the source code to
compile with ifort or gfortran.
@swails
Copy link
Collaborator Author

swails commented Sep 9, 2015

Paul Nerenberg says that this change is okay with you, @jayponder, so I'm going to go ahead and merge it.

swails added a commit that referenced this pull request Sep 9, 2015
Small fix to OpenMM torsion assignment
@swails swails merged commit 2568b12 into TinkerTools:master Sep 9, 2015
@swails swails deleted the omm_tors branch September 9, 2015 02:01
@pnerenberg
Copy link

I didn't say this! (It would be good to hear from @jayponder on this issue.) But I do agree that these are necessary changes for successful nucleic acid simulations. And the compiler check is a common sense item.

@pnerenberg
Copy link

@swails I just tried re-compiling on Stampede with gcc 4.9.1 and that #ifdef statement doesn't work.

First there is a warning:

gfortran -c -O3 -ffast-math -fopenmp initial.f -o initial.o
Warning: initial.f:80: Illegal preprocessor directive
Warning: initial.f:83: Illegal preprocessor directive

And then an error:

gfortran -O3 -ffast-math -fopenmp -static-libgcc -o alchemy.x alchemy.o libtinker.a -L/work/02555/psn/tinker/fftw/lib -lfftw3_threads -lfftw3; strip alchemy.x
libtinker.a(initial.o): In function initial_': initial.f:(.text+0x6d): undefined reference tokmp_set_stacksize_s_'
initial.f:(.text+0x79): undefined reference to kmp_set_blocktime_' /usr/bin/ld: link errors found, deleting executablealchemy.x'
collect2: error: ld returned 1 exit status
strip: 'alchemy.x': No such file
make: *** [alchemy.x] Error 1
make: *** Waiting for unfinished jobs....

@swails
Copy link
Collaborator Author

swails commented Oct 9, 2015

Yea, yea... I know. I found that out when I tried compiling it the other day. The problem is that C preprocessor directives are not processed on .f or .f90 files. They are only processed on .F and .F90 files. So if we rename initial.f to initial.F, it will work (but then we have to update the name in all of the build scripts, which is why I haven't done it yet).

I'll add it to my to-do list if @jayponder likes the idea.

@swails
Copy link
Collaborator Author

swails commented Oct 9, 2015

I didn't say this! (It would be good to hear from @jayponder on this issue.) But I do agree that these are necessary changes for successful nucleic acid simulations. And the compiler check is a common sense item.

I completely forget the context here. Sorry for throwing you under the bus if that's what happened... In any case, I merged this a long time ago and it only affects OpenMM (and fixes a bug).

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.

2 participants