Apply changes suggested during review

olla/include/openmm/kernels.h

@@ -55,7 +55,7 @@
 #include "openmm/KernelImpl.h"
 #include "openmm/LangevinIntegrator.h"
 #include "openmm/MonteCarloBarostat.h"
-#include "openmm/NoseHooverChain.h"
+#include "openmm/internal/NoseHooverChain.h"
 #include "openmm/PeriodicTorsionForce.h"
 #include "openmm/RBTorsionForce.h"
 #include "openmm/RMSDForce.h"
@@ -1060,7 +1060,7 @@ public:
 };
 
 /**
- * This kernel is invoked by VelocityVerletIntegrator to take one time step.
+ * This kernel is invoked by NoseHooverIntegrator to take one time step.
  */
 class IntegrateVelocityVerletStepKernel : public KernelImpl {
 public:
@@ -1073,14 +1073,14 @@ public:
      * Initialize the kernel.
      * 
      * @param system     the System this kernel will be applied to
-     * @param integrator the VelocityVerletIntegrator this kernel will be used for
+     * @param integrator the NoseHooverIntegrator this kernel will be used for
      */
     virtual void initialize(const System& system, const NoseHooverIntegrator& integrator) = 0;
     /**
      * Execute the kernel.
      * 
      * @param context    the context in which to execute this kernel
-     * @param integrator the VelocityVerletIntegrator this kernel is being used for
+     * @param integrator the NoseHooverIntegrator this kernel is being used for
      * @param forcesAreValid a reference to the parent integrator's boolean for keeping
      *                       track of the validity of the current forces.
      */
@@ -1089,7 +1089,7 @@ public:
      * Compute the kinetic energy.
      * 
      * @param context    the context in which to execute this kernel
-     * @param integrator the VelocityVerletIntegrator this kernel is being used for
+     * @param integrator the NoseHooverIntegrator this kernel is being used for
      */
     virtual double computeKineticEnergy(ContextImpl& context, const NoseHooverIntegrator& integrator) = 0;
 };

openmmapi/include/OpenMM.h

@@ -61,7 +61,6 @@
 #include "openmm/MonteCarloBarostat.h"
 #include "openmm/MonteCarloMembraneBarostat.h"
 #include "openmm/NonbondedForce.h"
-#include "openmm/NoseHooverChain.h"
 #include "openmm/Context.h"
 #include "openmm/OpenMMException.h"
 #include "openmm/PeriodicTorsionForce.h"

openmmapi/include/openmm/NoseHooverIntegrator.h

@@ -35,7 +35,7 @@
 #include "Integrator.h"
 #include "openmm/State.h"
 #include "openmm/Kernel.h"
-#include "openmm/NoseHooverChain.h"
+#include "internal/NoseHooverChain.h"
 #include "internal/windowsExport.h"
 
 namespace OpenMM {
@@ -58,15 +58,15 @@ public:
     /**
      * Create a NoseHooverIntegrator.
      *
-     * @param stepSize the step size with which to integrate the system (in picoseconds)
      * @param temperature the target temperature for the system.
      * @param collisionFrequency the frequency of the interaction with the heat bath (in 1/ps).
+     * @param stepSize the step size with which to integrate the system (in picoseconds)
      * @param chainLength the number of beads in the Nose-Hoover chain.
      * @param numMTS the number of step in the  multiple time step chain propagation algorithm.
      * @param numYoshidaSuzuki the number of terms in the Yoshida-Suzuki multi time step decomposition
      *        used in the chain propagation algorithm (must be 1, 3, or 5).
      */
-    explicit NoseHooverIntegrator(double stepSize, double temperature, int collisionFrequnency,
+    explicit NoseHooverIntegrator(double temperature, int collisionFrequnency, double stepSize, 
                                   int chainLength = 3, int numMTS = 3, int numYoshidaSuzuki = 3);
 
     virtual ~NoseHooverIntegrator();
@@ -99,9 +99,9 @@ public:
      * @param thermostatedPairs a list of pairs of connected atoms whose absolute center of mass motion
      *        and motion relative to one another will be independently thermostated.
      * @param temperature the target temperature for each pair's absolute of center of mass motion.
-     * @param relativeTemperature the target temperature for each pair's relative motion.
      * @param collisionFrequency the frequency of the interaction with the heat bath for the
      *        pairs' center of mass motion (in 1/ps).
+     * @param relativeTemperature the target temperature for each pair's relative motion.
      * @param relativeCollisionFrequency the frequency of the interaction with the heat bath for the
      *        pairs' relative motion (in 1/ps).
      * @param chainLength the number of beads in the Nose-Hoover chain.
@@ -111,8 +111,8 @@ public:
      */
      int addSubsystemThermostat(const std::vector<int>& thermostatedParticles,
                                 const std::vector< std::pair< int, int> >& thermostatedPairs,
-                                double temperature, double relativeTemperature,
-                                double collisionFrequency, double relativeCollisionFrequency,
+                                double temperature, double collisionFrequency, double relativeTemperature,
+                                double relativeCollisionFrequency,
                                 int chainLength = 3, int numMTS = 3, int numYoshidaSuzuki = 3);
     /**
      * Get the temperature of the i-th chain for controling absolute particle motion (in Kelvin).

openmmapi/include/openmm/NoseHooverChain.h → openmmapi/include/openmm/internal/NoseHooverChain.h

@@ -32,11 +32,11 @@
  * USE OR OTHER DEALINGS IN THE SOFTWARE.                                     *
  * -------------------------------------------------------------------------- */
 
-#include "Force.h"
+#include "openmm/Force.h"
 #include <string>
 #include <vector>
 #include "openmm/OpenMMException.h"
-#include "internal/windowsExport.h"
+#include "windowsExport.h"
 
 namespace OpenMM {
 

openmmapi/src/NoseHooverChain.cpp

@@ -29,7 +29,7 @@
  * USE OR OTHER DEALINGS IN THE SOFTWARE.                                     *
  * -------------------------------------------------------------------------- */
 
-#include "openmm/NoseHooverChain.h"
+#include "openmm/internal/NoseHooverChain.h"
 #include "openmm/OpenMMException.h"
 
 using namespace OpenMM;

openmmapi/src/NoseHooverIntegrator.cpp

@@ -33,10 +33,11 @@
 #include "openmm/Context.h"
 #include "openmm/Force.h"
 #include "openmm/System.h"
-#include "openmm/NoseHooverChain.h"
+#include "openmm/internal/NoseHooverChain.h"
 #include "openmm/OpenMMException.h"
 #include "openmm/CMMotionRemover.h"
 #include "openmm/internal/ContextImpl.h"
+#include "openmm/internal/AssertionUtilities.h"
 #include "openmm/kernels.h"
 #include <iostream>
 #include <string>
@@ -52,7 +53,7 @@ NoseHooverIntegrator::NoseHooverIntegrator(double stepSize):
     setStepSize(stepSize);
     setConstraintTolerance(1e-5);
 }
-NoseHooverIntegrator::NoseHooverIntegrator(double stepSize, double temperature, int collisionFrequnency,
+NoseHooverIntegrator::NoseHooverIntegrator(double temperature, int collisionFrequnency, double stepSize,
                                            int chainLength, int numMTS, int numYoshidaSuzuki) : forcesAreValid(false) {
     setStepSize(stepSize);
     setConstraintTolerance(1e-5);
@@ -69,14 +70,14 @@ std::pair<double, double> NoseHooverIntegrator::propagateChain(std::pair<double,
 int NoseHooverIntegrator::addThermostat(double temperature, double collisionFrequency,
                                         int chainLength, int numMTS, int numYoshidaSuzuki) {
 
-    return addSubsystemThermostat(std::vector<int>(), std::vector<std::pair<int, int>>(), temperature, temperature,
-                                  collisionFrequency, collisionFrequency, chainLength, numMTS, numYoshidaSuzuki);
+    return addSubsystemThermostat(std::vector<int>(), std::vector<std::pair<int, int>>(), temperature,
+                                  collisionFrequency, temperature, collisionFrequency, chainLength, numMTS, numYoshidaSuzuki);
 }
 
 int NoseHooverIntegrator::addSubsystemThermostat(const std::vector<int>& thermostatedParticles,
                                                  const std::vector< std::pair< int, int> > &thermostatedPairs,
-                                                 double temperature, double relativeTemperature,
-                                                 double collisionFrequency, double relativeCollisionFrequency,
+                                                 double temperature, double collisionFrequency,
+                                                 double relativeTemperature, double relativeCollisionFrequency,
                                                  int chainLength, int numMTS, int numYoshidaSuzuki) {
     auto data = ThermostatData(thermostatedParticles, thermostatedPairs, temperature,
                                relativeTemperature, collisionFrequency,
@@ -195,82 +196,50 @@ void NoseHooverIntegrator::createThermostats(const System &system) {
 }
 
 double NoseHooverIntegrator::getTemperature(int chainID) const {
-    if (chainID >= noseHooverChains.size()) {
-        throw OpenMMException("Cannot get temperature for chainID " + std::to_string(chainID)
-                + ". Only " + std::to_string(noseHooverChains.size()) + " have been registered with this integrator."
-        );
-    }
-    return noseHooverChains.at(chainID).getDefaultTemperature();
+    ASSERT_VALID_INDEX(chainID, noseHooverChains);
+    return noseHooverChains[chainID].getDefaultTemperature();
 }
 
 void NoseHooverIntegrator::setTemperature(double temperature, int chainID){
-    if (chainID >= noseHooverChains.size()) {
-        throw OpenMMException("Cannot set temperature for chainID " + std::to_string(chainID)
-                + ". Only " + std::to_string(noseHooverChains.size()) + " have been registered with this integrator."
-        );
-    }
-    noseHooverChains.at(chainID).setDefaultTemperature(temperature);
+    ASSERT_VALID_INDEX(chainID, noseHooverChains);
+    noseHooverChains[chainID].setDefaultTemperature(temperature);
 
 }
 
 double NoseHooverIntegrator::getRelativeTemperature(int chainID) const {
-    if (chainID >= noseHooverChains.size()) {
-        throw OpenMMException("Cannot get relative temperature for chainID " + std::to_string(chainID)
-                + ". Only " + std::to_string(noseHooverChains.size()) + " have been registered with this integrator."
-        );
-    }
-    return noseHooverChains.at(chainID).getDefaultRelativeTemperature();
+    ASSERT_VALID_INDEX(chainID, noseHooverChains);
+    return noseHooverChains[chainID].getDefaultRelativeTemperature();
 }
 
 void NoseHooverIntegrator::setRelativeTemperature(double temperature, int chainID){
-    if (chainID >= noseHooverChains.size()) {
-        throw OpenMMException("Cannot set relative temperature for chainID " + std::to_string(chainID)
-                + ". Only " + std::to_string(noseHooverChains.size()) + " have been registered with this integrator."
-        );
-    }
-    noseHooverChains.at(chainID).setDefaultRelativeTemperature(temperature);
+    ASSERT_VALID_INDEX(chainID, noseHooverChains);
+    noseHooverChains[chainID].setDefaultRelativeTemperature(temperature);
 
 }
 
 double NoseHooverIntegrator::getCollisionFrequency(int chainID) const {
-    if (chainID >= noseHooverChains.size()) {
-        throw OpenMMException("Cannot get collision frequency for chainID " + std::to_string(chainID)
-                + ". Only " + std::to_string(noseHooverChains.size()) + " have been registered with this integrator."
-        );
-    }
-    return noseHooverChains.at(chainID).getDefaultCollisionFrequency();
+    ASSERT_VALID_INDEX(chainID, noseHooverChains);
+    return noseHooverChains[chainID].getDefaultCollisionFrequency();
 }
 
 void NoseHooverIntegrator::setCollisionFrequency(double frequency, int chainID){
-    if (chainID >= noseHooverChains.size()) {
-        throw OpenMMException("Cannot set collision frequency for chainID " + std::to_string(chainID)
-                + ". Only " + std::to_string(noseHooverChains.size()) + " have been registered with this integrator."
-        );
-    }
-    noseHooverChains.at(chainID).setDefaultCollisionFrequency(frequency);
+    ASSERT_VALID_INDEX(chainID, noseHooverChains);
+    noseHooverChains[chainID].setDefaultCollisionFrequency(frequency);
 }
 
 double NoseHooverIntegrator::getRelativeCollisionFrequency(int chainID) const {
-    if (chainID >= noseHooverChains.size()) {
-        throw OpenMMException("Cannot get relative collision frequency for chainID " + std::to_string(chainID)
-                + ". Only " + std::to_string(noseHooverChains.size()) + " have been registered with this integrator."
-        );
-    }
-    return noseHooverChains.at(chainID).getDefaultRelativeCollisionFrequency();
+    ASSERT_VALID_INDEX(chainID, noseHooverChains);
+    return noseHooverChains[chainID].getDefaultRelativeCollisionFrequency();
 }
 
 void NoseHooverIntegrator::setRelativeCollisionFrequency(double frequency, int chainID){
-    if (chainID >= noseHooverChains.size()) {
-        throw OpenMMException("Cannot set relative collision frequency for chainID " + std::to_string(chainID)
-                + ". Only " + std::to_string(noseHooverChains.size()) + " have been registered with this integrator."
-        );
-    }
-    noseHooverChains.at(chainID).setDefaultRelativeCollisionFrequency(frequency);
+    ASSERT_VALID_INDEX(chainID, noseHooverChains);
+    noseHooverChains[chainID].setDefaultRelativeCollisionFrequency(frequency);
 }
 
 double NoseHooverIntegrator::computeKineticEnergy() {
     double kE = 0.0;
-    if(noseHooverChains.size()) {
+    if(noseHooverChains.size() > 0) {
         for (const auto &nhc: noseHooverChains){
             kE += nhcKernel.getAs<NoseHooverChainKernel>().computeMaskedKineticEnergy(*context, nhc, true).first;
         }
@@ -308,7 +277,7 @@ void NoseHooverIntegrator::initialize(ContextImpl& contextRef) {
             for(int particle = 0; particle < system.getNumParticles(); ++particle) {
                 double mass = system.getParticleMass(particle);
                 if ( (mass > 0) && (mass < 0.8) ){
-                    std::cout << "Warning: Found particles with mass between 0.0 and 0.8 dalton. Did you mean to make a DrudeVelocityVerletIntegrator instead? "
+                    std::cout << "Warning: Found particles with mass between 0.0 and 0.8 dalton. Did you mean to make a DrudeNoseHooverIntegrator instead? "
                                  "The thermostat you are about to use will not treat these particles as Drude particles!" << std::endl;
                 }
                 if(system.getParticleMass(particle) > 0) {

platforms/cuda/include/CudaKernels.h

@@ -1367,7 +1367,7 @@ private:
 };
 
 /*
- * This kernel is invoked by VelocityVerletIntegrator to take one time step.
+ * This kernel is invoked by NoseHooverIntegrator to take one time step.
  */
 class CudaIntegrateVelocityVerletStepKernel : public IntegrateVelocityVerletStepKernel {
 public:
@@ -1378,7 +1378,7 @@ public:
      * Initialize the kernel.
      * 
      * @param system     the System this kernel will be applied to
-     * @param integrator the VelocityVerletIntegrator this kernel will be used for
+     * @param integrator the NoseHooverIntegrator this kernel will be used for
      */
     void initialize(const System& system, const NoseHooverIntegrator& integrator);
     /**
@@ -1394,7 +1394,7 @@ public:
      * Compute the kinetic energy.
      * 
      * @param context    the context in which to execute this kernel
-     * @param integrator the VelocityVerletIntegrator this kernel is being used for
+     * @param integrator the NoseHooverIntegrator this kernel is being used for
      */
     double computeKineticEnergy(ContextImpl& context, const NoseHooverIntegrator& integrator);
 private:

platforms/reference/include/ReferenceKernels.h

@@ -1136,7 +1136,7 @@ private:
 }
 ;
 /**
- * This kernel is invoked by VelocityVerletIntegrator to take one time step.
+ * This kernel is invoked by NoseHooverIntegrator to take one time step.
  */
 class ReferenceIntegrateVelocityVerletStepKernel : public IntegrateVelocityVerletStepKernel {
 public:
@@ -1148,7 +1148,7 @@ public:
      * Initialize the kernel.
      * 
      * @param system     the System this kernel will be applied to
-     * @param integrator the VelocityVerletIntegrator this kernel will be used for
+     * @param integrator the NoseHooverIntegrator this kernel will be used for
      */
     void initialize(const System& system, const NoseHooverIntegrator& integrator);
     /**
@@ -1164,7 +1164,7 @@ public:
      * Compute the kinetic energy.
      * 
      * @param context    the context in which to execute this kernel
-     * @param integrator the VelocityVerletIntegrator this kernel is being used for
+     * @param integrator the NoseHooverIntegrator this kernel is being used for
      */
     double computeKineticEnergy(ContextImpl& context, const NoseHooverIntegrator& integrator);
 private:

plugins/drude/openmmapi/include/openmm/DrudeNoseHooverIntegrator.h

@@ -54,18 +54,18 @@ public:
     /**
      * Create a DrudeNoseHooverIntegrator.
      *
-     * @param stepSize       the step size with which to integrator the system (in picoseconds)
      * @param temperature the target temperature for the system.
-     * @param drudeTemperature the target temperature for the Drude particles, relative to their parent atom.
      * @param collisionFrequency the frequency of the system's interaction with the heat bath (in 1/ps).
+     * @param drudeTemperature the target temperature for the Drude particles, relative to their parent atom.
      * @param drudeCollisionFrequency the frequency of the drude particles' interaction with the heat bath (in 1/ps).
+     * @param stepSize       the step size with which to integrator the system (in picoseconds)
      * @param chainLength the number of beads in the Nose-Hoover chain.
      * @param numMTS the number of step in the  multiple time step chain propagation algorithm.
      * @param numYoshidaSuzuki the number of terms in the Yoshida-Suzuki multi time step decomposition
      *        used in the chain propagation algorithm (must be 1, 3, or 5).
      */
-    DrudeNoseHooverIntegrator(double stepSize, double temperature, double drudeTemperature,
-                              double collisionFrequency, double drudeCollisionFrequency,
+    DrudeNoseHooverIntegrator(double temperature, double collisionFrequency, 
+                              double drudeTemperature, double drudeCollisionFrequency, double stepSize, 
                               int chainLength = 3, int numMTS = 3, int numYoshidaSuzuki = 3);
 
     virtual ~DrudeNoseHooverIntegrator();

plugins/drude/openmmapi/src/DrudeNoseHooverIntegrator.cpp

@@ -46,13 +46,13 @@ using namespace OpenMM;
 using std::string;
 using std::vector;
 
-DrudeNoseHooverIntegrator::DrudeNoseHooverIntegrator(double stepSize, double temperature, double drudeTemperature,
-                                                     double collisionFrequency, double drudeCollisionFrequency,
-                                                     int chainLength, int numMTS, int numYoshidaSuzuki) :
+DrudeNoseHooverIntegrator::DrudeNoseHooverIntegrator(double temperature, double collisionFrequency, 
+                                                     double drudeTemperature, double drudeCollisionFrequency,
+                                                     double stepSize, int chainLength, int numMTS, int numYoshidaSuzuki) :
     NoseHooverIntegrator(stepSize) {
 
     addSubsystemThermostat(std::vector<int>(), std::vector<std::pair<int, int>>(), temperature,
-                           drudeTemperature, collisionFrequency, drudeCollisionFrequency,
+                           collisionFrequency, drudeTemperature, drudeCollisionFrequency,
                            chainLength, numMTS, numYoshidaSuzuki);
 }
 

plugins/drude/tests/TestDrudeNoseHoover.h

@@ -30,7 +30,7 @@
  * -------------------------------------------------------------------------- */
 
 #include "openmm/internal/AssertionUtilities.h"
-#include "openmm/NoseHooverChain.h"
+#include "openmm/internal/NoseHooverChain.h"
 #include "openmm/CMMotionRemover.h"
 #include "openmm/DrudeNoseHooverIntegrator.h"
 #include "openmm/Context.h"
@@ -120,8 +120,9 @@ void testWaterBox(Platform& platform){
     double frequency = 100.0;
     double frequencyDrude = 80.0;
     int randomSeed = 100;
-    DrudeNoseHooverIntegrator integ(0.0005, temperature, temperatureDrude,
-                                    frequency, frequencyDrude, chainLength, numMTS, numYS);;
+    DrudeNoseHooverIntegrator integ(temperature, frequency, 
+                                    temperatureDrude, frequencyDrude, 0.0005, 
+                                    chainLength, numMTS, numYS);
     Context context(system, integ, platform);
     context.setPositions(positions);
     context.setVelocitiesToTemperature(temperature, randomSeed);
@@ -179,7 +180,7 @@ void testWaterBox(Platform& platform){
     }
     totalKE /= numSteps;
     ASSERT_USUALLY_EQUAL_TOL(temperature, meanTemp, 0.03);
-    ASSERT_USUALLY_EQUAL_TOL(temperatureDrude, meanDrudeTemp, 0.03);
+    ASSERT_USUALLY_EQUAL_TOL(temperatureDrude, meanDrudeTemp, 0.04);
 }
 
 int main(int argc, char* argv[]) {

tests/TestNoseHooverThermostat.h

@@ -30,7 +30,7 @@
  * -------------------------------------------------------------------------- */
 
 #include "openmm/internal/AssertionUtilities.h"
-#include "openmm/NoseHooverChain.h"
+#include "openmm/internal/NoseHooverChain.h"
 #include "openmm/NoseHooverIntegrator.h"
 #include "openmm/Context.h"
 #include "openmm/State.h"
@@ -162,7 +162,7 @@ void testDimerBox(bool constrain=true) {
     int numMTS = 3;
     int numYS = 3;
     int chainLength = 5;
-    auto integrator = simpleConstruct ? NoseHooverIntegrator(0.001, temperature, collisionFrequency, chainLength, numMTS, numYS)
+    auto integrator = simpleConstruct ? NoseHooverIntegrator(temperature, collisionFrequency, 0.001, chainLength, numMTS, numYS)
                                       : NoseHooverIntegrator(0.001);
     if (!simpleConstruct)
         integrator.addThermostat(temperature, collisionFrequency, chainLength, numMTS, numYS);
@@ -225,9 +225,9 @@ void testCheckpoints() {
     double kineticEnergy = 1e6;
     double temperature=300, collisionFrequency=1, chainLength=3, numMTS=3, numYS=3;
     chainLength = 10;
-    integrator.addSubsystemThermostat(std::vector<int>(), std::vector<std::pair<int,int>>{{0,1}},  temperature, temperature, collisionFrequency, collisionFrequency,
+    integrator.addSubsystemThermostat(std::vector<int>(), std::vector<std::pair<int,int>>{{0,1}},  temperature, collisionFrequency, temperature, collisionFrequency,
                                       chainLength, numMTS, numYS);
-    newIntegrator.addSubsystemThermostat(std::vector<int>(), std::vector<std::pair<int,int>>{{0,1}},  temperature, temperature, collisionFrequency, collisionFrequency,
+    newIntegrator.addSubsystemThermostat(std::vector<int>(), std::vector<std::pair<int,int>>{{0,1}},  temperature, collisionFrequency, temperature, collisionFrequency,
                                       chainLength, numMTS, numYS);
     Context context(system, integrator, platform);
     Context newContext(system, newIntegrator, platform);