Improved usability of grompp by rejecting more invalid input
authorMark Abraham <mark.j.abraham@gmail.com>
Mon, 26 Aug 2019 08:59:22 +0000 (10:59 +0200)
committerPaul Bauer <paul.bauer.q@gmail.com>
Wed, 28 Aug 2019 08:33:11 +0000 (10:33 +0200)
Improved test coverage and code documentation.

Also renamed some test cases so people understand the intent better.

Change-Id: I06f704d2d5516888ef8c84e78b68fa8c53fa7948

12 files changed:
docs/release-notes/2020/major/tools.rst
src/gromacs/fileio/readinp.cpp
src/gromacs/gmxpreprocess/tests/readir.cpp
src/gromacs/gmxpreprocess/tests/refdata/GetIrTest_AcceptsDefineParametersWithValuesIncludingAssignment.xml [moved from src/gromacs/gmxpreprocess/tests/refdata/GetIrTest_DefineHandlesAssignmentOnRhs.xml with 100% similarity]
src/gromacs/gmxpreprocess/tests/refdata/GetIrTest_AcceptsElectricField.xml [moved from src/gromacs/gmxpreprocess/tests/refdata/GetIrTest_ProducesOutputFromElectricField.xml with 100% similarity]
src/gromacs/gmxpreprocess/tests/refdata/GetIrTest_AcceptsElectricFieldOscillating.xml [moved from src/gromacs/gmxpreprocess/tests/refdata/GetIrTest_ProducesOutputFromElectricFieldOscillating.xml with 100% similarity]
src/gromacs/gmxpreprocess/tests/refdata/GetIrTest_AcceptsElectricFieldPulsed.xml [moved from src/gromacs/gmxpreprocess/tests/refdata/GetIrTest_ProducesOutputFromElectricFieldPulsed.xml with 100% similarity]
src/gromacs/gmxpreprocess/tests/refdata/GetIrTest_AcceptsEmptyLines.xml [moved from src/gromacs/gmxpreprocess/tests/refdata/GetIrTest_EmptyInputWorks.xml with 100% similarity]
src/gromacs/gmxpreprocess/tests/refdata/GetIrTest_AcceptsImplicitSolventNo.xml [moved from src/gromacs/gmxpreprocess/tests/refdata/GetIrTest_ImplicitSolventNoWorks.xml with 100% similarity]
src/gromacs/gmxpreprocess/tests/refdata/GetIrTest_AcceptsKeyWithoutValue.xml [moved from src/gromacs/gmxpreprocess/tests/refdata/GetIrTest_UserErrorsSilentlyTolerated.xml with 100% similarity]
src/gromacs/gmxpreprocess/tests/refdata/GetIrTest_AcceptsMimic.xml [moved from src/gromacs/gmxpreprocess/tests/refdata/GetIrTest_HandlesMimic.xml with 100% similarity]
src/gromacs/gmxpreprocess/tests/refdata/GetIrTest_HandlesDifferentKindsOfMdpLines.xml

index f07f7624600c6a8ba2c7db5a5f8ad361b16e67b6..ed1226a5ef277624a4b91b42fe185a7bb12535f4 100644 (file)
@@ -12,3 +12,15 @@ Fixed bug in gmx order -calcdist
 The reference position for the distance calculation was calculated
 wrongly.
 
+Improved grompp usability by rejecting more invalid .mdp lines
+""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""
+Lines like
+
+    ref-t 298
+    = 0.1
+    =
+
+are now all rejected with a descriptive message, which will help
+prevent some kinds of errors in constructing .mdp inputs. Note that an
+.mdp parameter name with a missing value is still accepted, and leads
+to the default behavior for that parameter.
index 0945a2ef48e0abe87cb9c699ad89a0144d316152..743fd325555f0fc85aa53ab57a4a961d99e37e0b 100644 (file)
@@ -85,11 +85,8 @@ read_inpfile(gmx::TextInputStream *stream, const char *fn,
         auto tokens = gmx::splitAndTrimDelimitedString(line, '=');
         if (tokens.size() < 2)
         {
-            // TODO this seems like it silently ignores the user accidentally deleting an equals sign...
-            if (debug)
-            {
-                fprintf(debug, "No = on line %d in file %s, ignored\n", indexOfLineReadFromFile, fn);
-            }
+            auto message = gmx::formatString("No '=' to separate .mdp parameter key and value was found on line:\n'%s'", line.c_str());
+            warning_error(wi, message);
             continue;
         }
         if (tokens.size() > 2)
@@ -104,22 +101,25 @@ read_inpfile(gmx::TextInputStream *stream, const char *fn,
             auto firstEqualsPos = line.find('=');
             tokens.emplace_back(gmx::stripString(line.substr(firstEqualsPos + 1)));
         }
+        GMX_RELEASE_ASSERT(tokens.size() == 2, "Must have tokens for key and value");
+        if (tokens[0].empty() && tokens[1].empty())
+        {
+            auto message = gmx::formatString("No .mdp parameter name or value was found on line:\n'%s'", line.c_str());
+            warning_error(wi, message);
+            continue;
+        }
         if (tokens[0].empty())
         {
-            // TODO ignoring such lines does not seem like good behaviour
-            if (debug)
-            {
-                fprintf(debug, "Empty left hand side on line %d in file %s, ignored\n", indexOfLineReadFromFile, fn);
-            }
+            auto message = gmx::formatString("No .mdp parameter name was found on the left-hand side of '=' on line:\n'%s'", line.c_str());
+            warning_error(wi, message);
             continue;
         }
         if (tokens[1].empty())
         {
-            // TODO ignoring such lines does not seem like good behaviour
-            if (debug)
-            {
-                fprintf(debug, "Empty right hand side on line %d in file %s, ignored\n", indexOfLineReadFromFile, fn);
-            }
+            // Users are probably using this for lines like
+            //   tcoupl = ;v-rescale
+            //   comm-grps =
+            // so we accept their intent to use the default behavior.
             continue;
         }
 
@@ -136,7 +136,7 @@ read_inpfile(gmx::TextInputStream *stream, const char *fn,
         {
             auto message = gmx::formatString("Parameter \"%s\" doubly defined\n",
                                              tokens[0].c_str());
-            warning_error(wi, message.c_str());
+            warning_error(wi, message);
         }
     }
     /* This preserves the behaviour of the old code, which issues some
index 634a08291ca47f4bc63b452d0f9d4070b1dadfa3..ada23298276cec401f91e8567e7b2967fb076e6b 100644 (file)
@@ -123,11 +123,13 @@ TEST_F(GetIrTest, HandlesDifferentKindsOfMdpLines)
     const char *inputMdpFile[] = {
         "; File to run my simulation",
         "title = simulation",
+        "define = -DBOOLVAR -DVAR=VALUE",
         ";",
         "xtc_grps = System ; was Protein",
         "include = -I/home/me/stuff",
         "",
         "tau-t = 0.1 0.3",
+        "ref-t = ;290 290",
         "tinit = 0.3",
         "init_step = 0",
         "nstcomm = 100",
@@ -145,19 +147,32 @@ TEST_F(GetIrTest, HandlesOnlyCutoffScheme)
     runTest(inputMdpFile);
 }
 
-// TODO Stop accepting any of these
-TEST_F(GetIrTest, UserErrorsSilentlyTolerated)
+TEST_F(GetIrTest, RejectsNonCommentLineWithNoEquals)
 {
-    const char *inputMdpFile[] = {
-        "title simulation",
-        "xtc_grps = ",
-        "= -I/home/me/stuff",
-        "="
-    };
-    runTest(joinStrings(inputMdpFile, "\n"));
+    const char *inputMdpFile = "title simulation";
+    EXPECT_DEATH_IF_SUPPORTED(runTest(inputMdpFile), "No '=' to separate");
+}
+
+TEST_F(GetIrTest, AcceptsKeyWithoutValue)
+{
+    // Users are probably using lines like this
+    const char *inputMdpFile = "xtc_grps = ";
+    runTest(inputMdpFile);
+}
+
+TEST_F(GetIrTest, RejectsValueWithoutKey)
+{
+    const char *inputMdpFile = "= -I/home/me/stuff";
+    EXPECT_DEATH_IF_SUPPORTED(runTest(inputMdpFile), "No .mdp parameter name was found");
+}
+
+TEST_F(GetIrTest, RejectsEmptyKeyAndEmptyValue)
+{
+    const char *inputMdpFile = " = ";
+    EXPECT_DEATH_IF_SUPPORTED(runTest(inputMdpFile), "No .mdp parameter name or value was found");
 }
 
-TEST_F(GetIrTest, DefineHandlesAssignmentOnRhs)
+TEST_F(GetIrTest, AcceptsDefineParametersWithValuesIncludingAssignment)
 {
     const char *inputMdpFile[] = {
         "define = -DBOOL -DVAR=VALUE",
@@ -165,7 +180,7 @@ TEST_F(GetIrTest, DefineHandlesAssignmentOnRhs)
     runTest(joinStrings(inputMdpFile, "\n"));
 }
 
-TEST_F(GetIrTest, EmptyInputWorks)
+TEST_F(GetIrTest, AcceptsEmptyLines)
 {
     const char *inputMdpFile = "";
     runTest(inputMdpFile);
@@ -173,43 +188,43 @@ TEST_F(GetIrTest, EmptyInputWorks)
 
 // These tests observe how the electric-field keys behave, since they
 // are currently the only ones using the new Options-style handling.
-TEST_F(GetIrTest, ProducesOutputFromElectricField)
+TEST_F(GetIrTest, AcceptsElectricField)
 {
     const char *inputMdpFile = "electric-field-x = 1.2 0 0 0";
     runTest(inputMdpFile);
 }
 
-TEST_F(GetIrTest, ProducesOutputFromElectricFieldPulsed)
+TEST_F(GetIrTest, AcceptsElectricFieldPulsed)
 {
     const char *inputMdpFile = "electric-field-y = 3.7 2.0 6.5 1.0";
     runTest(inputMdpFile);
 }
 
-TEST_F(GetIrTest, ProducesOutputFromElectricFieldOscillating)
+TEST_F(GetIrTest, AcceptsElectricFieldOscillating)
 {
     const char *inputMdpFile = "electric-field-z = 3.7 7.5 0 0";
     runTest(inputMdpFile);
 }
 
-TEST_F(GetIrTest, TerminatesOnDuplicateOldAndNewKeys)
+TEST_F(GetIrTest, RejectsDuplicateOldAndNewKeys)
 {
     const char *inputMdpFile[] = {"verlet-buffer-drift = 1.3", "verlet-buffer-tolerance = 2.7"};
     EXPECT_DEATH_IF_SUPPORTED(runTest(joinStrings(inputMdpFile, "\n")), "A parameter is present with both");
 }
 
-TEST_F(GetIrTest, ImplicitSolventNoWorks)
+TEST_F(GetIrTest, AcceptsImplicitSolventNo)
 {
     const char *inputMdpFile = "implicit-solvent = no";
     runTest(inputMdpFile);
 }
 
-TEST_F(GetIrTest, ImplicitSolventYesWorks)
+TEST_F(GetIrTest, RejectsImplicitSolventYes)
 {
     const char *inputMdpFile = "implicit-solvent = yes";
     EXPECT_DEATH_IF_SUPPORTED(runTest(inputMdpFile), "Invalid enum");
 }
 
-TEST_F(GetIrTest, HandlesMimic)
+TEST_F(GetIrTest, AcceptsMimic)
 {
     const char *inputMdpFile[] = {"integrator = mimic", "QMMM-grps = QMatoms"};
     runTest(joinStrings(inputMdpFile, "\n"));
index e643904413af1eb697e61e1f3cf29359e561b8c2..dddb943d272184e37fdef50ad06c52cd948f4c5b 100644 (file)
@@ -8,7 +8,7 @@
 ; e.g.: -I/home/joe/doe -I/home/mary/roe
 include                  = -I/home/me/stuff
 ; e.g.: -DPOSRES -DFLEXIBLE (note these variable names are case sensitive)
-define                   = 
+define                   = -DBOOLVAR -DVAR=VALUE
 
 ; RUN CONTROL PARAMETERS
 integrator               = steep