From 6b68a103f5c7a05abeb5e6ac85f313aed9afc52c Mon Sep 17 00:00:00 2001
From: Owen Arnold <owen.arnold@stfc.ac.uk>
Date: Mon, 2 Sep 2019 16:49:16 +0100
Subject: [PATCH] File production seems to be OK

Main change here is preventing LoadNexusProcessed from running the
instrument save when going via the ESS route.

New problem is that LoadNexusProcessed does not attempt to look for new
Geometry in Nexus as part of the loading, and therefore gives up when it
shouldn't

LoadNexusProcessed also annoyinly looking for group named "instrument",
which we now do not adhere to.
---
 Framework/API/inc/MantidAPI/ExperimentInfo.h  |  2 +-
 Framework/API/src/ExperimentInfo.cpp          |  7 ++--
 .../MantidDataHandling/LoadNexusProcessed.h   |  3 +-
 .../inc/MantidDataHandling/SaveNexusESS.h     |  1 +
 .../MantidDataHandling/SaveNexusProcessed.h   | 10 +++---
 .../DataHandling/src/LoadNexusProcessed.cpp   | 25 ++++++++++---
 Framework/DataHandling/src/SaveNexusESS.cpp   |  9 ++++-
 .../DataHandling/src/SaveNexusProcessed.cpp   | 22 +++++++-----
 .../DataHandling/test/SaveNexusESSTest.h      | 36 ++++++++++++++++---
 .../inc/MantidTestHelpers/FileResource.h      | 12 +++++--
 10 files changed, 99 insertions(+), 28 deletions(-)

diff --git a/Framework/API/inc/MantidAPI/ExperimentInfo.h b/Framework/API/inc/MantidAPI/ExperimentInfo.h
index c3946648a93..8964b807e40 100644
--- a/Framework/API/inc/MantidAPI/ExperimentInfo.h
+++ b/Framework/API/inc/MantidAPI/ExperimentInfo.h
@@ -118,7 +118,7 @@ public:
   void saveExperimentInfoNexus(::NeXus::File *file) const;
   /// Saves this experiment description to the open NeXus file
   void saveExperimentInfoNexus(::NeXus::File *file, bool saveInstrument,
-                               bool saveSample, bool saveLogs) const;
+                               bool saveSample, bool saveLogs, bool saveLegacyInstrument=true) const;
   /// Loads an experiment description from the open NeXus file
   void loadExperimentInfoNexus(const std::string &nxFilename,
                                ::NeXus::File *file, std::string &parameterStr);
diff --git a/Framework/API/src/ExperimentInfo.cpp b/Framework/API/src/ExperimentInfo.cpp
index f983d0958a3..70254c848ab 100644
--- a/Framework/API/src/ExperimentInfo.cpp
+++ b/Framework/API/src/ExperimentInfo.cpp
@@ -1149,9 +1149,12 @@ void ExperimentInfo::invalidateAllSpectrumDefinitions() {
 /** Save the object to an open NeXus file.
  * @param file :: open NeXus file
  */
-void ExperimentInfo::saveExperimentInfoNexus(::NeXus::File *file) const {
+void ExperimentInfo::saveExperimentInfoNexus(::NeXus::File *file,
+                                             bool saveLegacyInstrument) const {
   Instrument_const_sptr instrument = getInstrument();
-  instrument->saveNexus(file, "instrument");
+  if (saveLegacyInstrument) {
+    instrument->saveNexus(file, "instrument");
+  }
   sample().saveNexus(file, "sample");
   run().saveNexus(file, "logs");
 }
diff --git a/Framework/DataHandling/inc/MantidDataHandling/LoadNexusProcessed.h b/Framework/DataHandling/inc/MantidDataHandling/LoadNexusProcessed.h
index ae7d3842491..7c7a455bebd 100644
--- a/Framework/DataHandling/inc/MantidDataHandling/LoadNexusProcessed.h
+++ b/Framework/DataHandling/inc/MantidDataHandling/LoadNexusProcessed.h
@@ -97,7 +97,8 @@ private:
   API::Workspace_sptr loadEntry(Mantid::NeXus::NXRoot &root,
                                 const std::string &entry_name,
                                 const double &progressStart,
-                                const double &progressRange);
+                                const double &progressRange,
+                                const std::string &filename);
 
   API::Workspace_sptr loadTableEntry(Mantid::NeXus::NXEntry &entry);
 
diff --git a/Framework/DataHandling/inc/MantidDataHandling/SaveNexusESS.h b/Framework/DataHandling/inc/MantidDataHandling/SaveNexusESS.h
index 7fd155689a5..9d33a933080 100644
--- a/Framework/DataHandling/inc/MantidDataHandling/SaveNexusESS.h
+++ b/Framework/DataHandling/inc/MantidDataHandling/SaveNexusESS.h
@@ -35,6 +35,7 @@ protected:
 private:
   void saveNexusGeometry(const Mantid::API::MatrixWorkspace &ws,
                          const std::string &filename) override;
+  virtual bool saveLegacyInstrument() override;
   void init() override;
   void exec() override;
 };
diff --git a/Framework/DataHandling/inc/MantidDataHandling/SaveNexusProcessed.h b/Framework/DataHandling/inc/MantidDataHandling/SaveNexusProcessed.h
index aa8469b93de..53ecc2abc31 100644
--- a/Framework/DataHandling/inc/MantidDataHandling/SaveNexusProcessed.h
+++ b/Framework/DataHandling/inc/MantidDataHandling/SaveNexusProcessed.h
@@ -63,14 +63,16 @@ public:
       const std::vector<int> &wsIndices,
       const ::NeXus::NXcompression compression = ::NeXus::LZW) const;
 
-protected:
-  /// Override process groups
-  bool processGroups() override;
-
   virtual void saveNexusGeometry(const Mantid::API::MatrixWorkspace &,
                                  const std::string &){
       /* Do nothing by default */};
 
+  virtual bool saveLegacyInstrument() { return true; }
+
+protected:
+  /// Override process groups
+  bool processGroups() override;
+
   /// Overwrites Algorithm method.
   void init() override;
   /// Overwrites Algorithm method
diff --git a/Framework/DataHandling/src/LoadNexusProcessed.cpp b/Framework/DataHandling/src/LoadNexusProcessed.cpp
index 4e474279b7e..043ff5648f0 100644
--- a/Framework/DataHandling/src/LoadNexusProcessed.cpp
+++ b/Framework/DataHandling/src/LoadNexusProcessed.cpp
@@ -32,11 +32,14 @@
 #include "MantidKernel/UnitFactory.h"
 #include "MantidNexus/NexusClasses.h"
 #include "MantidNexus/NexusFileIO.h"
+#include "MantidNexusGeometry/AbstractLogger.h"
+#include "MantidNexusGeometry/NexusGeometryParser.h"
 
 #include <boost/regex.hpp>
 #include <boost/shared_array.hpp>
 #include <boost/shared_ptr.hpp>
 
+#include <H5Cpp.h>
 #include <nexus/NeXusException.hpp>
 
 #include <map>
@@ -374,7 +377,8 @@ void LoadNexusProcessed::exec() {
   progress(0, "Opening file...");
 
   // Throws an approriate exception if there is a problem with file access
-  NXRoot root(getPropertyValue("Filename"));
+  const std::string filename = getPropertyValue("Filename");
+  NXRoot root(filename);
 
   // "Open" the same file but with the C++ interface
   m_cppFile = new ::NeXus::File(root.m_fileID);
@@ -406,7 +410,7 @@ void LoadNexusProcessed::exec() {
 
   // Take the first real workspace obtainable. We need it even if loading
   // groups.
-  API::Workspace_sptr tempWS = loadEntry(root, targetEntryName, 0, 1);
+  API::Workspace_sptr tempWS = loadEntry(root, targetEntryName, 0, 1, filename);
 
   if (nWorkspaceEntries == 1 || !bDefaultEntryNumber) {
     // We have what we need.
@@ -488,7 +492,7 @@ void LoadNexusProcessed::exec() {
         local_workspace =
             loadEntry(root, basename + indexStr,
                       static_cast<double>(p - 1) / nWorkspaceEntries_d,
-                      1. / nWorkspaceEntries_d);
+                      1. / nWorkspaceEntries_d, filename);
       }
 
       declareProperty(std::make_unique<WorkspaceProperty<API::Workspace>>(
@@ -1434,12 +1438,14 @@ API::MatrixWorkspace_sptr LoadNexusProcessed::loadNonEventEntry(
  * for this entry
  * @param progressRange :: The percentage range that the progress reporting
  * should cover
+ * @param filename :: name of loaded File
  * @returns A 2D workspace containing the loaded data
  */
 API::Workspace_sptr LoadNexusProcessed::loadEntry(NXRoot &root,
                                                   const std::string &entry_name,
                                                   const double &progressStart,
-                                                  const double &progressRange) {
+                                                  const double &progressRange,
+                                                  const std::string &filename) {
   progress(progressStart, "Opening entry " + entry_name + "...");
 
   NXEntry mtd_entry = root.openEntry(entry_name);
@@ -1585,6 +1591,17 @@ API::Workspace_sptr LoadNexusProcessed::loadEntry(NXRoot &root,
     g_log.warning(e.what());
     g_log.warning("Try running LoadInstrument Algorithm on the Workspace to "
                   "update the geometry");
+    try {
+      using namespace Mantid::NexusGeometry;
+      auto instrument = NexusGeometry::NexusGeometryParser::createInstrument(
+          filename, NexusGeometry::makeLogger(&g_log));
+      local_workspace->setInstrument(
+          Geometry::Instrument_const_sptr(std::move(instrument)));
+    } catch (std::exception &e) {
+      g_log.warning(e.what());
+    } catch (H5::Exception &e) {
+      g_log.warning(e.getDetailMsg());
+    }
   }
 
   // Now assign the spectra-detector map
diff --git a/Framework/DataHandling/src/SaveNexusESS.cpp b/Framework/DataHandling/src/SaveNexusESS.cpp
index ab9e2b68964..47af11920b9 100644
--- a/Framework/DataHandling/src/SaveNexusESS.cpp
+++ b/Framework/DataHandling/src/SaveNexusESS.cpp
@@ -52,7 +52,7 @@ void SaveNexusESS::saveNexusGeometry(const Mantid::API::MatrixWorkspace &ws,
     NexusGeometry::LogAdapter<Kernel::Logger> adapter(&g_log);
     NexusGeometry::NexusGeometrySave::saveInstrument(
         ws.componentInfo(), ws.detectorInfo(), filename, "mantid_workspace_1",
-        adapter, false);
+        adapter, true);
   } catch (std::exception &e) {
     g_log.error(std::string(e.what()) +
                 " Nexus Geometry may be absent or incomplete "
@@ -64,6 +64,13 @@ void SaveNexusESS::saveNexusGeometry(const Mantid::API::MatrixWorkspace &ws,
   }
 }
 
+bool SaveNexusESS::saveLegacyInstrument() {
+  /*A hard No on this one. Mantids's current NXDetector, NXMonitor ... types do
+   * not have information needed for loading and just cause down-stream
+   * problems. Best not to save them in the first place.*/
+  return false;
+}
+
 //----------------------------------------------------------------------------------------------
 /** Initialize the algorithm's properties.
  */
diff --git a/Framework/DataHandling/src/SaveNexusProcessed.cpp b/Framework/DataHandling/src/SaveNexusProcessed.cpp
index 3377de9ac7a..a9a4d403f03 100644
--- a/Framework/DataHandling/src/SaveNexusProcessed.cpp
+++ b/Framework/DataHandling/src/SaveNexusProcessed.cpp
@@ -240,7 +240,7 @@ bool SaveNexusProcessed::doExec(
   // write instrument data, if present and writer enabled
   if (matrixWorkspace) {
     // Save the instrument names, ParameterMap, sample, run
-    matrixWorkspace->saveExperimentInfoNexus(&cppFile);
+    matrixWorkspace->saveExperimentInfoNexus(&cppFile, saveLegacyInstrument());
     prog_init.reportIncrement(1, "Writing sample and instrument");
 
     // check if all X() are in fact the same array
@@ -268,14 +268,18 @@ bool SaveNexusProcessed::doExec(
           workspaceTypeGroupName.c_str(), true);
     }
 
-    cppFile.openGroup("instrument", "NXinstrument");
-    cppFile.makeGroup("detector", "NXdetector", true);
-    cppFile.putAttr("version", 1);
-    saveSpectraDetectorMapNexus(*matrixWorkspace, &cppFile, indices,
-                                ::NeXus::LZW);
-    saveSpectrumNumbersNexus(*matrixWorkspace, &cppFile, indices, ::NeXus::LZW);
-    cppFile.closeGroup();
-    cppFile.closeGroup();
+    if (saveLegacyInstrument()) {
+      cppFile.openGroup("instrument", "NXinstrument");
+      cppFile.makeGroup("detector", "NXdetector", true);
+
+      cppFile.putAttr("version", 1);
+      saveSpectraDetectorMapNexus(*matrixWorkspace, &cppFile, indices,
+                                  ::NeXus::LZW);
+      saveSpectrumNumbersNexus(*matrixWorkspace, &cppFile, indices,
+                               ::NeXus::LZW);
+      cppFile.closeGroup();
+      cppFile.closeGroup();
+    }
 
   } // finish matrix workspace specifics
 
diff --git a/Framework/DataHandling/test/SaveNexusESSTest.h b/Framework/DataHandling/test/SaveNexusESSTest.h
index 7a1fb570c0e..964173534e3 100644
--- a/Framework/DataHandling/test/SaveNexusESSTest.h
+++ b/Framework/DataHandling/test/SaveNexusESSTest.h
@@ -9,7 +9,9 @@
 
 #include <cxxtest/TestSuite.h>
 
+#include "MantidDataHandling/LoadNexusProcessed.h"
 #include "MantidDataHandling/SaveNexusESS.h"
+#include "MantidDataHandling/SaveNexusProcessed.h"
 #include "MantidGeometry/Instrument.h"
 #include "MantidGeometry/Instrument/ComponentInfo.h"
 #include "MantidGeometry/Instrument/DetectorInfo.h"
@@ -19,7 +21,8 @@
 #include "MantidTestHelpers/WorkspaceCreationHelper.h"
 #include <boost/filesystem.hpp>
 #include <memory>
-using Mantid::DataHandling::SaveNexusESS;
+
+using namespace Mantid::DataHandling;
 
 class SaveNexusESSTest : public CxxTest::TestSuite {
 public:
@@ -40,15 +43,15 @@ public:
     alg.isExecuted();
   }
 
-  void test_Init() {
+  void xtest_Init() {
     SaveNexusESS alg;
     TS_ASSERT_THROWS_NOTHING(alg.initialize());
     TS_ASSERT(alg.isInitialized())
   }
 
-  void test_exec_instrument_details() {
+  void test_exec_rectangular_instrument_details() {
     using namespace Mantid::NexusGeometry;
-    const ScopedFileHandle fileInfo("test_rectangular.nxs");
+    ScopedFileHandle fileInfo("test_rectangular.nxs");
     auto ws =
         WorkspaceCreationHelper::create2DWorkspaceWithRectangularInstrument(
             1 /*numBanks*/, 10 /*numPixels*/, 10 /*numBins*/);
@@ -73,6 +76,31 @@ public:
     // Rectangular detector bank. Hence subtranction from output.
     TS_ASSERT_EQUALS(outCompInfo->size(), inCompInfo.size() - 10);
   }
+
+  void test_exec_rectangular_data() {
+    ScopedFileHandle fileInfo("test_rectangular_data.nxs");
+    auto wsIn =
+        WorkspaceCreationHelper::create2DWorkspaceWithRectangularInstrument(
+            1 /*numBanks*/, 10 /*numPixels*/, 12 /*numBins*/);
+
+    do_execute(fileInfo.fullPath(), wsIn);
+
+    LoadNexusProcessed loader;
+    loader.setChild(true);
+    loader.setRethrows(true);
+    loader.initialize();
+    loader.setProperty("Filename", fileInfo.fullPath());
+    loader.setPropertyValue("OutputWorkspace", "dummy");
+    loader.execute();
+
+    Mantid::API::Workspace_sptr wsOut = loader.getProperty("OutputWorkspace");
+    auto matrixWSOut =
+        boost::dynamic_pointer_cast<Mantid::API::MatrixWorkspace>(wsOut);
+
+    TS_ASSERT_EQUALS(matrixWSOut->blocksize(), 12);
+    TS_ASSERT_EQUALS(matrixWSOut->getNumberHistograms(), 10 * 10);
+    TS_ASSERT(matrixWSOut->detectorInfo().isEquivalent(wsIn->detectorInfo()));
+  }
 };
 
 #endif /* MANTID_DATAHANDLING_SAVENEXUSESSTEST_H_ */
diff --git a/Framework/TestHelpers/inc/MantidTestHelpers/FileResource.h b/Framework/TestHelpers/inc/MantidTestHelpers/FileResource.h
index c5b0bb68f80..f2939d2adf4 100644
--- a/Framework/TestHelpers/inc/MantidTestHelpers/FileResource.h
+++ b/Framework/TestHelpers/inc/MantidTestHelpers/FileResource.h
@@ -17,12 +17,14 @@
 #define MANTID_NEXUSGEOMETRY_FILERESOURCE_H_
 
 #include <boost/filesystem.hpp>
+#include <iostream>
 #include <string>
 
 class ScopedFileHandle {
 
 public:
-  ScopedFileHandle(const std::string &fileName) {
+  ScopedFileHandle(const std::string &fileName, bool debugMode = false)
+      : m_debugMode(debugMode) {
 
     const auto temp_dir = boost::filesystem::temp_directory_path();
     auto temp_full_path = temp_dir;
@@ -41,17 +43,23 @@ public:
     }
   }
 
+  void setDebugMode(bool mode) { m_debugMode = mode; }
   std::string fullPath() const { return m_full_path.generic_string(); }
 
   ~ScopedFileHandle() {
 
     // file is removed at end of file handle's lifetime
     if (boost::filesystem::is_regular_file(m_full_path)) {
-      boost::filesystem::remove(m_full_path);
+      if (!m_debugMode)
+        boost::filesystem::remove(m_full_path);
+      else
+        std::cout << "Debug file at: " << m_full_path << " not removed. "
+                  << std::endl;
     }
   }
 
 private:
+  bool m_debugMode;
   boost::filesystem::path m_full_path; // full path to file
 
   // prevent heap allocation for ScopedFileHandle
-- 
GitLab