Merge branch 'integration' into devel

Jürgen Knödlseder
2 parents ed2f1398 0401c237
Showing 6 changed files with 240 additions and 142 deletions
ChangeLog
README.md
configure.ac
doc/source/admin/release_history/1.6.2.rst
doc/source/admin/release_history/index.rst
src/support/ctool.cpp
@@ -4,6 +4,14 @@
         ========================
  
  
+2019-06-25
+
+        * Bug fix version 1.6.2 released
+        ================================
+
+        Fix OpenMP issue in ctbin when using energy bounds from file (#2912)
+
+
 2019-06-05
  
         * Bug fix version 1.6.1 released
 ctools information
 ==================
 * Version:             1.7.0.dev (29 May 2019)
-* Author:              Juergen Knoedlseder (jurgen.knodlseder@irap.omp.eu)
 * GammaLib dependency: 1.7.0.dev
  
 [![Build Status](https://cta-jenkins.irap.omp.eu/buildStatus/icon?job=ctools-integrate-os)](https://cta-jenkins.irap.omp.eu/job/ctools-integrate-os/)
 #############################################################################
 # Configuration for ctools - tools for science analysis of CTA data         #
 # ------------------------------------------------------------------------- #
-# Copyright (C) 2011-2018 Juergen Knoedlseder                               #
+# Copyright (C) 2011-2019 Juergen Knoedlseder                               #
 # ------------------------------------------------------------------------- #
 #                                                                           #
 #  This program is free software: you can redistribute it and/or modify     #
+.. _1.6.2:
+
+ctools 1.6.2 release (25 June 2019)
+===================================
+
+Overview
+--------
+
+ctools 1.6.2 is a bug fix release for ctools release 1.6.
+
+The following bugs were fixed:
+
+* [`2912 <https://cta-redmine.irap.omp.eu/issues/2912>`_] -
+  Fix OpenMP issue in ctbin when using energy bounds from file
@@ -7,6 +7,7 @@ Release History
    :maxdepth: 1
  
    ctools 1.7 <1.7>
+   ctools 1.6.2 <1.6.2>
    ctools 1.6.1 <1.6.1>
    ctools 1.6 <1.6>
    ctools 1.5.4 <1.5.4>
@@ -586,70 +586,105 @@ GEbounds ctool::create_ebounds(void)
         // Get filename
         GFilename ebinfile = (*this)["ebinfile"].filename();
  
-        // If no extension name was provided then use default extension names
-        if (!ebinfile.has_extname()) {
-
-            // Open energy boundary file using the EBOUNDS or ENERGYBINS
-            // extension or energy values using the ENERGIES extension.
-            // Throw an exception if opening fails.
-            GFits file(ebinfile.url());
-            if (file.contains(gammalib::extname_ebounds)) {
-                file.close();
-                ebounds.load(ebinfile);
-            }
-            else if (file.contains("ENERGYBINS")) {
-                file.close();
-                ebinfile = ebinfile.url() + "[ENERGYBINS]";
-                ebounds.load(ebinfile);
-            }
-            else if (file.contains(gammalib::extname_energies)) {
-                file.close();
-                ebounds.set(GEnergies(ebinfile));
-            }
-            else {
-                file.close();
-                std::string msg = "No extension with name \""+
-                                  gammalib::extname_ebounds+"\", "
-                                  "\"ENERGYBINS\" or \""+
-                                  gammalib::extname_energies+"\" found in FITS "
-                                  "file \""+ebinfile+"\". Please specify a "
-                                  "valid energy binning file.";
-                throw GException::invalid_value(G_CREATE_EBOUNDS, msg);
-            }
-        }
+        // Initialise exception message string
+        std::string msg;
+
+        // Make loading of energy boundaries OpenMP thread save. The critical
+        // region is put into an exception block to prevent throwing exceptions
+        // within the critical block
+        #pragma omp critical(ctool_create_ebounds)
+        {
+            try {
  
-        // ... otherwise load energy boundaries from filename including
-        // extension
-        else {
+                // If no extension name was provided then use default extension
+                // names
+                if (!ebinfile.has_extname()) {
+
+                    // Open energy boundary file using the EBOUNDS or ENERGYBINS
+                    // extension or energy values using the ENERGIES extension.
+                    // If opening fails then set exception message. The
+                    // exception is thrown later outside the critical OpenMP
+                    // block.
+                    GFits file(ebinfile.url());
+                    if (file.contains(gammalib::extname_ebounds)) {
+                        file.close();
+                        ebounds.load(ebinfile);
+                    }
+                    else if (file.contains("ENERGYBINS")) {
+                        file.close();
+                        ebinfile = ebinfile.url() + "[ENERGYBINS]";
+                        ebounds.load(ebinfile);
+                    }
+                    else if (file.contains(gammalib::extname_energies)) {
+                        file.close();
+                        ebounds.set(GEnergies(ebinfile));
+                    }
+                    else {
+                        file.close();
+                        msg = "No extension with name \""+
+                              gammalib::extname_ebounds+"\", "
+                              "\"ENERGYBINS\" or \""+
+                              gammalib::extname_energies+"\" found in FITS "
+                              "file \""+ebinfile+"\". Please specify a "
+                              "valid energy binning file.";
+                    }
  
-            // Open energy boundary file
-            GFits file(ebinfile.url());
-
-            // If FITS file does not contain requested extension then throw
-            // an exception
-            if (!file.contains(ebinfile.extname())) {
-                std::string msg = "No extension \""+ebinfile.extname()+"\" "
-                                  "found in energy binning file \""+
-                                  ebinfile.url()+"\". Please provide a valid "
-                                  "extension name.";
-                throw GException::invalid_value(G_CREATE_EBOUNDS, msg);
-            }
+                } // endif: no extension name specified
  
-            // Get table extension
-            GFitsTable& table = *file.table(ebinfile.extname());
+                // ... otherwise load energy boundaries from filename including
+                // extension
+                else {
  
-            // If table contains one column then load energies, otherwise load
-            // energy boundaries.
-            if (table.ncols() == 1) {
-                file.close();
-                ebounds.set(GEnergies(ebinfile));
-            }
-            else {
-                file.close();
-                ebounds.load(ebinfile);
+                    // Open energy boundary file
+                    GFits file(ebinfile.url());
+
+                    // If FITS file does not contain requested extension then
+                    // set exception message. The exception is thrown later
+                    // outside the critical OpenMP zone
+                    if (!file.contains(ebinfile.extname())) {
+                        msg = "No extension \""+ebinfile.extname()+"\" "
+                              "found in energy binning file \""+
+                              ebinfile.url()+"\". Please provide a valid "
+                              "extension name.";
+                    }
+
+                    // ... otherwise load energy boundaries
+                    else {
+
+                        // Get table extension
+                        GFitsTable& table = *file.table(ebinfile.extname());
+
+                        // If table contains one column then load energies,
+                        // otherwise load energy boundaries.
+                        if (table.ncols() == 1) {
+                            file.close();
+                            ebounds.set(GEnergies(ebinfile));
+                        }
+                        else {
+                            file.close();
+                            ebounds.load(ebinfile);
+                        }
+
+                    } // endelse: extension name was present in FITS file
+
+                } // endelse: loaded energy boundaries from table extension
+
+            } // endtry
+
+            // Catch any exceptions that occured in OpenMP critical block
+            // and recover the error message
+            catch (const std::exception& e) {
+                msg = e.what();
             }
  
-        } // endelse: loaded energy boundaries from table extension
+        } // end critical OpenMP block
+
+        // If we have an exception message then throw an exception now. We
+        // have to do this since we should not throw exceptions inside OpenMP
+        // critical blocks
+        if (!msg.empty()) {
+            throw GException::invalid_value(G_CREATE_EBOUNDS, msg);
+        }
  
     } // endif: ebinalg was "FILE"
  
@@ -717,70 +752,105 @@ GEnergies ctool::create_energies(void)
         // Get filename
         GFilename ebinfile = (*this)["ebinfile"].filename();
  
-        // If no extension name was provided then use default extension names
-        if (!ebinfile.has_extname()) {
+        // Initialise exception message string
+        std::string msg;
  
-            // Open energy boundary file using the EBOUNDS or ENERGYBINS
-            // extension or energy values using the ENERGIES extension.
-            // Throw an exception if opening fails.
-            GFits file(ebinfile.url());
-            if (file.contains(gammalib::extname_energies)) {
-                file.close();
-                energies.load(ebinfile);
-            }
-            else if (file.contains(gammalib::extname_ebounds)) {
-                file.close();
-                energies.set(GEbounds(ebinfile));
-            }
-            else if (file.contains("ENERGYBINS")) {
-                file.close();
-                ebinfile = ebinfile.url() + "[ENERGYBINS]";
-                energies.set(GEbounds(ebinfile));
-            }
-            else {
-                file.close();
-                std::string msg = "No extension with name \""+
-                                  gammalib::extname_energies+"\", "
-                                  "\"ENERGYBINS\" or \""+
-                                  gammalib::extname_ebounds+"\" found in FITS "
-                                  "file \""+ebinfile+"\". Please specify a "
-                                  "valid energy binning file.";
-                throw GException::invalid_value(G_CREATE_ENERGIES, msg);
-            }
-        }
+        // Make loading of energies OpenMP thread save. The critical region is
+        // put into an exception block to prevent throwing exceptions within
+        // the critical block
+        #pragma omp critical(ctool_create_energies)
+        {
+            try {
  
-        // ... otherwise load energy boundaries from filename including
-        // extension
-        else {
+                // If no extension name was provided then use default extension
+                // names
+                if (!ebinfile.has_extname()) {
+
+                    // Open energy boundary file using the EBOUNDS or ENERGYBINS
+                    // extension or energy values using the ENERGIES extension.
+                    // If opening fails then set exception message. The
+                    // exception is thrown later outside the critical OpenMP
+                    // block.
+                    GFits file(ebinfile.url());
+                    if (file.contains(gammalib::extname_energies)) {
+                        file.close();
+                        energies.load(ebinfile);
+                    }
+                    else if (file.contains(gammalib::extname_ebounds)) {
+                        file.close();
+                        energies.set(GEbounds(ebinfile));
+                    }
+                    else if (file.contains("ENERGYBINS")) {
+                        file.close();
+                        ebinfile = ebinfile.url() + "[ENERGYBINS]";
+                        energies.set(GEbounds(ebinfile));
+                    }
+                    else {
+                        file.close();
+                        msg = "No extension with name \""+
+                              gammalib::extname_energies+"\", "
+                              "\"ENERGYBINS\" or \""+
+                              gammalib::extname_ebounds+"\" found in FITS "
+                              "file \""+ebinfile+"\". Please specify a "
+                              "valid energy binning file.";
+                    }
  
-            // Open energy boundary file
-            GFits file(ebinfile.url());
-
-            // If FITS file does not contain requested extension then throw
-            // an exception
-            if (!file.contains(ebinfile.extname())) {
-                std::string msg = "No extension \""+ebinfile.extname()+"\" "
-                                  "found in energy node file \""+
-                                  ebinfile.url()+"\". Please provide a valid "
-                                  "extension name.";
-                throw GException::invalid_value(G_CREATE_ENERGIES, msg);
-            }
+                }
  
-            // Get table extension
-            GFitsTable& table = *file.table(ebinfile.extname());
+                // ... otherwise load energy boundaries from filename including
+                // extension
+                else {
  
-            // If table contains one column then load energies, otherwise
-            // load energy boundaries.
-            if (table.ncols() == 1) {
-                file.close();
-                energies.load(ebinfile);
-            }
-            else {
-                file.close();
-                energies.set(GEbounds(ebinfile));
+                    // Open energy boundary file
+                    GFits file(ebinfile.url());
+
+                    // If FITS file does not contain requested extension then
+                    // set exception message. The exception is thrown later
+                    // outside the critical OpenMP zone
+                    if (!file.contains(ebinfile.extname())) {
+                    msg = "No extension \""+ebinfile.extname()+"\" "
+                          "found in energy node file \""+
+                          ebinfile.url()+"\". Please provide a valid "
+                          "extension name.";
+                    }
+
+                    // ... otherwise load energies
+                    else {
+
+                        // Get table extension
+                        GFitsTable& table = *file.table(ebinfile.extname());
+
+                        // If table contains one column then load energies,
+                        // otherwise load energy boundaries.
+                        if (table.ncols() == 1) {
+                            file.close();
+                            energies.load(ebinfile);
+                        }
+                        else {
+                            file.close();
+                            energies.set(GEbounds(ebinfile));
+                        }
+
+                    } // endelse: extension name was present in FITS file
+
+                } // endelse: loaded energies from table extension
+
+            } // endtry
+
+            // Catch any exceptions that occured in OpenMP critical block
+            // and recover the error message
+            catch (const std::exception& e) {
+                msg = e.what();
             }
  
-        } // endelse: loaded energy boundaries from table extension
+        } // end critical OpenMP block
+
+        // If we have an exception message then throw an exception now. We
+        // have to do this since we should not throw exceptions inside OpenMP
+        // critical blocks
+        if (!msg.empty()) {
+            throw GException::invalid_value(G_CREATE_ENERGIES, msg);
+        }
  
     } // endif: ebinalg was "FILE"
  
@@ -2203,34 +2273,40 @@ void ctool::save_event_list(const GCTAObservation* obs,
             }
         }
  
-        // Create output FITS file
-        GFits outfits;
-
-        // Write observation into FITS file
-        obs->write(outfits, outevt, outgti);
-
-        // Copy all extensions other than evtname and gtiname extensions
-        // from the input to the output event list. The evtname and
-        // gtiname extensions are written by the save method, all others
-        // that may eventually be present have to be copied over
-        // explicitly.
-        GFits infits(infile);
-        for (int extno = 1; extno < infits.size(); ++extno) {
-            GFitsHDU* hdu = infits.at(extno);
-            if (hdu->extname() != evtname &&
-                hdu->extname() != gtiname &&
-                hdu->extname() != outevt  &&
-                hdu->extname() != outgti) {
-                outfits.append(*hdu);
+        // Make writing of FITS file OpenMP thread save
+        #pragma omp critical(ctool_save_event_list)
+        {
+
+            // Create output FITS file
+            GFits outfits;
+
+            // Write observation into FITS file
+            obs->write(outfits, outevt, outgti);
+
+            // Copy all extensions other than evtname and gtiname extensions
+            // from the input to the output event list. The evtname and
+            // gtiname extensions are written by the save method, all others
+            // that may eventually be present have to be copied over
+            // explicitly.
+            GFits infits(infile);
+            for (int extno = 1; extno < infits.size(); ++extno) {
+                GFitsHDU* hdu = infits.at(extno);
+                if (hdu->extname() != evtname &&
+                    hdu->extname() != gtiname &&
+                    hdu->extname() != outevt  &&
+                    hdu->extname() != outgti) {
+                    outfits.append(*hdu);
+                }
             }
-        }
  
-        // Close input file
-        infits.close();
+            // Close input file
+            infits.close();
+
+            // Save file to disk and close it (we need both operations)
+            outfits.saveto(outname.url(), clobber());
+            outfits.close();
  
-        // Save file to disk and close it (we need both operations)
-        outfits.saveto(outname.url(), clobber());
-        outfits.close();
+        } // end: omp critical section
  
     } // endif: observation was unbinned