From db77e659417c4a61fc0c44e9dd8c6e8cacba1775 Mon Sep 17 00:00:00 2001
From: Anders Kvellestad <anders.kvellestad@fys.uio.no>
Date: Fri, 9 Feb 2024 13:14:49 +0100
Subject: [PATCH 1/4] Trying to fix faulty if-else logic related resuming jobs
 with the hdf5_v1 printer.

---
 .../src/printers/hdf5printer/hdf5printer.cpp  | 82 ++++++++++---------
 1 file changed, 43 insertions(+), 39 deletions(-)

diff --git a/Printers/src/printers/hdf5printer/hdf5printer.cpp b/Printers/src/printers/hdf5printer/hdf5printer.cpp
index 65792494a5..5b588d8882 100644
--- a/Printers/src/printers/hdf5printer/hdf5printer.cpp
+++ b/Printers/src/printers/hdf5printer/hdf5printer.cpp
@@ -420,50 +420,54 @@ namespace Gambit
           #endif
           if(HDF5::checkFileReadable(finalfile, msg_finalfile))
           {
-            if(overwrite_file and not get_resume())
+            // Note: "not resume" means "start or restart"
+            if(not get_resume())
             {
-              // Note: "not resume" means "start or restart"
-              // Delete existing output file
-              std::ostringstream command;
-              command << "rm -f "<<finalfile;
-              logger() << LogTags::printers << LogTags::info << "Running shell command: " << command.str() << EOM;
-              FILE* fp = popen(command.str().c_str(), "r");
-              if(fp==NULL)
-              {
-                // Error running popen
-                std::ostringstream errmsg;
-                errmsg << "rank "<<myRank<<": Error deleting existing output file (requested by 'delete_file_on_restart' printer option; target filename is "<<finalfile<<")! popen failed to run the command (command was '"<<command.str()<<"')";
-                printer_error().raise(LOCAL_INFO, errmsg.str());
-              }
-              else if(pclose(fp)!=0)
+              if(overwrite_file)
               {
-                // Command returned exit code!=0, or pclose failed
-                std::ostringstream errmsg;
-                errmsg << "rank "<<myRank<<": Error deleting existing output file (requested by 'delete_file_on_restart' printer option; target filename is "<<finalfile<<")! Shell command failed to executed successfully, see stderr (command was '"<<command.str()<<"').";
-                printer_error().raise(LOCAL_INFO, errmsg.str());
+                // Delete existing output file
+                std::ostringstream command;
+                command << "rm -f "<<finalfile;
+                logger() << LogTags::printers << LogTags::info << "Running shell command: " << command.str() << EOM;
+                FILE* fp = popen(command.str().c_str(), "r");
+                if(fp==NULL)
+                {
+                  // Error running popen
+                  std::ostringstream errmsg;
+                  errmsg << "rank "<<myRank<<": Error deleting existing output file (requested by 'delete_file_on_restart' printer option; target filename is "<<finalfile<<")! popen failed to run the command (command was '"<<command.str()<<"')";
+                  printer_error().raise(LOCAL_INFO, errmsg.str());
+                }
+                else if(pclose(fp)!=0)
+                {
+                  // Command returned exit code!=0, or pclose failed
+                  std::ostringstream errmsg;
+                  errmsg << "rank "<<myRank<<": Error deleting existing output file (requested by 'delete_file_on_restart' printer option; target filename is "<<finalfile<<")! Shell command failed to executed successfully, see stderr (command was '"<<command.str()<<"').";
+                  printer_error().raise(LOCAL_INFO, errmsg.str());
+                }
               }
-            }
-            else
-            {
-              // File exists, so check if 'group' is readable, and throw error if it exists
-              file_id = HDF5::openFile(finalfile);
-              std::string msg_group;
-              std::cout << "Group readable: " << finalfile << " , " << group << " : " << HDF5::checkGroupReadable(file_id, group, msg_group) << std::endl;
-              if(HDF5::checkGroupReadable(file_id, group, msg_group))
+              else
               {
-                // Group already exists, error!
-                std::ostringstream errmsg;
-                errmsg << "Error preparing pre-existing output file '"<<finalfile<<"' for writing via hdf5printer! The requested output group '"<<group<<" already exists in this file! Please take one of the following actions:"<<std::endl;
-                errmsg << "  1. Choose a new group via the 'group' option in the Printer section of your input YAML file;"<<std::endl;
-                errmsg << "  2. Delete the existing group from '"<<finalfile<<"';"<<std::endl;
-                errmsg << "  3. Delete the existing output file, or set 'delete_file_on_restart: true' in your input YAML file to give GAMBIT permission to automatically delete it (applies when -r/--restart flag used);"<<std::endl;
-                errmsg << std::endl;
-                errmsg << "*** Note: This error most commonly occurs when you try to resume a scan that has already finished! ***" <<std::endl;
-                errmsg << std::endl;
-                printer_error().raise(LOCAL_INFO, errmsg.str());
+                // File exists, so check if 'group' is readable, and throw error if it exists
+                file_id = HDF5::openFile(finalfile);
+                std::string msg_group;
+                std::cout << "Group readable: " << finalfile << " , " << group << " : " << HDF5::checkGroupReadable(file_id, group, msg_group) << std::endl;
+                if(HDF5::checkGroupReadable(file_id, group, msg_group))
+                {
+                  // Group already exists, error!
+                  std::ostringstream errmsg;
+                  errmsg << "Error preparing pre-existing output file '"<<finalfile<<"' for writing via hdf5printer! The requested output group '"<<group<<" already exists in this file! Please take one of the following actions:"<<std::endl;
+                  errmsg << "  1. Choose a new group via the 'group' option in the Printer section of your input YAML file;"<<std::endl;
+                  errmsg << "  2. Delete the existing group from '"<<finalfile<<"';"<<std::endl;
+                  errmsg << "  3. Delete the existing output file, or set 'delete_file_on_restart: true' in your input YAML file to give GAMBIT permission to automatically delete it (applies when -r/--restart flag used);"<<std::endl;
+                  errmsg << std::endl;
+                  errmsg << "*** Note: This error most commonly occurs when you try to resume a scan that has already finished! ***" <<std::endl;
+                  errmsg << std::endl;
+                  printer_error().raise(LOCAL_INFO, errmsg.str());
+                }
+                HDF5::closeFile(file_id);
+                exit(1);
               }
-              HDF5::closeFile(file_id);
-              exit(1);
+
             }
           }
 

From 727542e7b9ec7c474512e9bd99a5c891337c35ed Mon Sep 17 00:00:00 2001
From: Anders Kvellestad <anders.kvellestad@fys.uio.no>
Date: Fri, 9 Feb 2024 13:38:44 +0100
Subject: [PATCH 2/4] Moved comment in hdf5printer_v2.cpp

---
 Printers/src/printers/hdf5printer_v2/hdf5printer_v2.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Printers/src/printers/hdf5printer_v2/hdf5printer_v2.cpp b/Printers/src/printers/hdf5printer_v2/hdf5printer_v2.cpp
index fbb8fbbd92..3b366c69b6 100644
--- a/Printers/src/printers/hdf5printer_v2/hdf5printer_v2.cpp
+++ b/Printers/src/printers/hdf5printer_v2/hdf5printer_v2.cpp
@@ -1478,9 +1478,9 @@ namespace Gambit
                 std::string msg_finalfile;
                 if(HDF5::checkFileReadable(file, msg_finalfile))
                 {
+                    // Note: "not resume" means "start or restart"
                     if(not get_resume())
                     {
-                        // Note: "not resume" means "start or restart"
                         if(overwrite_file)
                         {
                             // Delete existing output file

From f561f7296bd0970be0efdeb8beb4fcdb7ffc94dc Mon Sep 17 00:00:00 2001
From: Anders Kvellestad <anders.kvellestad@fys.uio.no>
Date: Fri, 9 Feb 2024 14:43:44 +0100
Subject: [PATCH 3/4] Changed file suffix of expected combined hdf5 file, from
 '_combined_temp' to '_combined'.

This way a user don't risk deleting both the combined file and all the temporary files if they do e.g. 'rm *.hdf5_temp_*' after running the combine script.
---
 Printers/src/printers/hdf5printer/hdf5printer.cpp | 4 ++--
 Printers/standalone/manual_hdf5_combine.cpp       | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Printers/src/printers/hdf5printer/hdf5printer.cpp b/Printers/src/printers/hdf5printer/hdf5printer.cpp
index 5b588d8882..415d20cf27 100644
--- a/Printers/src/printers/hdf5printer/hdf5printer.cpp
+++ b/Printers/src/printers/hdf5printer/hdf5printer.cpp
@@ -389,7 +389,7 @@ namespace Gambit
 
         // Name of file where combined results from previous (unfinished) runs end up
         std::ostringstream rename;
-        rename << finalfile << "_temp_combined";
+        rename << finalfile << "_combined";
         tmp_comb_file = rename.str();
 
         // HDF5 group (virtual "folder") inside output file in which to store datasets
@@ -1089,7 +1089,7 @@ namespace Gambit
                                   // follow a fixed format and they all exist. We check for this before
                                   // running this function, so this should be fine.
 
-      // If we set the second last flag 'true' then Greg's code will assume that a '_temp_combined' output file
+      // If we set the second last flag 'true' then Greg's code will assume that a '_combined' output file
       // exists, and it will crash if it doesn't. So we need to first check if such a file exists.
       bool combined_file_exists = Utils::file_exists(tmp_comb_file); // We already check this externally; pass in as flag?
       logger() << LogTags::printers << LogTags::info << "combined_file_exists? " << combined_file_exists << EOM;
diff --git a/Printers/standalone/manual_hdf5_combine.cpp b/Printers/standalone/manual_hdf5_combine.cpp
index 4da0d2a14b..21a96201e8 100644
--- a/Printers/standalone/manual_hdf5_combine.cpp
+++ b/Printers/standalone/manual_hdf5_combine.cpp
@@ -61,7 +61,7 @@ void usage()
           "\n                         them from <filename>. Filenames are to be supplied in a single"
           "\n                         ascii file, on separate lines."
           "\n                         In this mode <filename> will only be used for naming the output"
-          "\n                         file (Note: WITHOUT the usual addition of '_temp_combined'!!!)."
+          "\n                         file (Note: WITHOUT the usual addition of '_combined'!!!)."
           "\n                         All files must have the same <group>."
           "\n                         Note: Auxilliary ('RA') datasets will be IGNORED! These should"
           "\n                         be combined during 'normal' combination of results from a single"
@@ -196,7 +196,7 @@ int main(int argc, char* argv[])
 
       // Name of temporary combined file, if one exists
       std::ostringstream name;
-      name << finalfile << "_temp_combined";
+      name << finalfile << "_combined";
       tmp_comb_file = name.str();
 
       combined_file_exists = Utils::file_exists(tmp_comb_file);

From fb18fabbb9c283c21e2900fcf23b4acdced7466e Mon Sep 17 00:00:00 2001
From: Anders Kvellestad <anders.kvellestad@fys.uio.no>
Date: Fri, 9 Feb 2024 15:14:48 +0100
Subject: [PATCH 4/4] Edited some error and log messages for clarity.

---
 .../src/printers/hdf5printer/hdf5printer.cpp    | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/Printers/src/printers/hdf5printer/hdf5printer.cpp b/Printers/src/printers/hdf5printer/hdf5printer.cpp
index 415d20cf27..e5c84fdbad 100644
--- a/Printers/src/printers/hdf5printer/hdf5printer.cpp
+++ b/Printers/src/printers/hdf5printer/hdf5printer.cpp
@@ -494,7 +494,7 @@ namespace Gambit
               {
                 // There is no combined output either, so disable resume mode
                 std::ostringstream msg;
-                msg<<"No temporary output from a previous scan found (or it is unreadable); this will be treated as a NEW scan";
+                msg<<"No combined output file " << tmp_comb_file << " from a previous scan found (or it is unreadable); this will be treated as a NEW scan";
                 std::cout<<msg.str()<<std::endl;
                 logger() << LogTags::info << msg.str();
                 set_resume(false);
@@ -537,7 +537,7 @@ namespace Gambit
             logger() << LogTags::printers << LogTags::info << "Rank " << myRank << ": tmp_comb_file readable? " << HDF5::checkFileReadable(tmp_comb_file) << "(filename: " << tmp_comb_file << ")" << EOM;
             if( HDF5::checkFileReadable(tmp_comb_file) )
             {
-              logger() << LogTags::info << "Scanning existing temporary combined output file, to prepare for adding new data" << EOM;
+              logger() << LogTags::info << "Scanning existing temporary combined output file " << tmp_comb_file << ", to prepare for adding new data" << EOM;
               // Open HDF5 file
               file_id = HDF5::openFile(tmp_comb_file);
 
@@ -748,7 +748,7 @@ namespace Gambit
       std::string msg;
       if( HDF5::checkFileReadable(tmp_comb_file, msg) )
       {
-        logger() << LogTags::repeat_to_cout << LogTags::info << "...Existing temporary combined output file was found and is readable" << EOM;
+        logger() << LogTags::repeat_to_cout << LogTags::info << "...Existing temporary combined output file " << tmp_comb_file << " was found and is readable" << EOM;
         combined_file_readable=true;
       }
       else
@@ -824,7 +824,10 @@ namespace Gambit
         else
         {
            std::ostringstream errmsg;
-           errmsg << " Process level temporary HDF5 output was detected, however the 'disable_combine_routines' option is set for the HDF5 printer plugin. The combine code is therefore not permitted to run, so this job cannot proceed. Please either manually combine the output files, restart the scan, or set this option to 'false'" << std::endl;
+           errmsg << " Process level temporary HDF5 output was detected, however the 'disable_combine_routines' option is set for the HDF5 printer plugin." 
+                  << " The combine code is therefore not permitted to run, so this run cannot proceed. "
+                  << " To resume the previous run, either manually combine the temp files into a file " << tmp_comb_file << " and delete/move all the temp files,"
+                  << " or set the 'disable_combine_routines' option to 'false'. To instead start a *new* scan, run gambit with the '-r' flag." << std::endl;
            printer_error().raise(LOCAL_INFO, errmsg.str());
         }
       }
@@ -1092,11 +1095,11 @@ namespace Gambit
       // If we set the second last flag 'true' then Greg's code will assume that a '_combined' output file
       // exists, and it will crash if it doesn't. So we need to first check if such a file exists.
       bool combined_file_exists = Utils::file_exists(tmp_comb_file); // We already check this externally; pass in as flag?
-      logger() << LogTags::printers << LogTags::info << "combined_file_exists? " << combined_file_exists << EOM;
+      logger() << LogTags::printers << LogTags::info << "tmp_comb_file (" << tmp_comb_file << ") exists? " << combined_file_exists << EOM;
       if(not combined_file_exists)
-        std::cout << "Combined file NOT found, combining now..." << std::endl;
+        std::cout << "Combined file " << tmp_comb_file << " NOT found, combining now..." << std::endl;
       else
-        std::cout << "Combined file found, skipping combination" << std::endl;
+        std::cout << "Combined file " << tmp_comb_file << " found, skipping combination" << std::endl;
       // Second last bool just tells the routine to delete the temporary files when it is done
       // Last flag, if false, tells routines to throw an error if any expected temporary file cannot be opened for any reason
       HDF5::combine_hdf5_files(tmp_comb_file, finalfile, group, metadata_group, num, combined_file_exists, true, false);