Refactor LoadHelper to use NeXus::File #38766

peterfpeterson · 2025-01-30T20:36:55Z

Modify LoadHelper::addNexusFieldsToWsRun() to take a ::NeXus::File rather than a napi NXhandle and update all users of the method. This is part of the consolidation of nexus code in mantid.

Refs #38332.

To test:

This is a refactor and all existing tests should continue to pass.

This does not require release notes because it is a refactor.

Reviewer

Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.

Code Review

Is the code of an acceptable quality?
Does the code conform to the coding standards?
Are the unit tests small and test the class in isolation?
If there is GUI work does it follow the GUI standards?
If there are changes in the release notes then do they describe the changes appropriately?
Do the release notes conform to the release notes guide?

Functional Tests

Do changes function as described? Add comments below that describe the tests performed?
Do the changes handle unexpected situations, e.g. bad input?
Has the relevant (user and developer) documentation been added/updated?

Does everything look good? Mark the review as Approve. A member of @mantidproject/gatekeepers will take care of it.

Gatekeeper

If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.

buildconfig/CMake/CppCheck_Suppressions.txt.in

github-actions · 2025-01-31T16:16:46Z

👋 Hi, @peterfpeterson,

Conflicts have been detected against the base branch. Please rebase your branch against the base branch.

rboston628

Still trying to figure out if LoadHelper::recurseAndAddNexusFieldsToWsRun is equivalent to the original. Here are some comments in the meantime. I'm not sure if the NeXus files close automatically when they go out of scope; if not, they need to be closed.

rboston628 · 2025-02-02T00:55:22Z

Framework/DataHandling/src/LoadILLTOF2.cpp

    g_log.debug() << "convertNexusToProperties: Error loading " << filename;
    throw Kernel::Exception::FileError("Unable to open File:", filename);
  }
-  LoadHelper::addNexusFieldsToWsRun(nxfileID, runDetails);
-  NXclose(&nxfileID);
+


The file needs to be closed

rboston628 · 2025-02-02T00:55:45Z

Framework/DataHandling/src/LoadILLSANS.cpp

-  NXstatus nxStat = NXopen(filename.c_str(), NXACC_READ, &nxHandle);
-  if (nxStat != NXstatus::NX_ERROR) {
+  try {
+    ::NeXus::File nxHandle(filename, NXACC_READ);


The file is opened but not closed.

rboston628 · 2025-02-02T00:56:02Z

Framework/DataHandling/src/LoadILLSALSA.cpp

-  NXhandle nxHandle;
-  NXopen(filename.c_str(), NXACC_READ, &nxHandle);
+
+  ::NeXus::File nxHandle(filename, NXACC_READ);


The file is opened but not closed.

rboston628 · 2025-02-02T00:56:18Z

Framework/DataHandling/src/LoadILLReflectometry.cpp

-  NXclose(&nxfileID);
+
+  try {
+    ::NeXus::File nxfileID(filename, NXACC_READ);


The file is opened but not closed.

rboston628 · 2025-02-02T00:56:31Z

Framework/DataHandling/src/LoadILLPolarizedDiffraction.cpp

-
-  if (nxStat != NXstatus::NX_ERROR) {
+  try {
+    ::NeXus::File nxHandle(m_fileName, NXACC_READ);


The file is opened but not closed/

rboston628 · 2025-02-02T00:56:42Z

Framework/DataHandling/src/LoadILLLagrange.cpp

-
-  if (nxStat != NXstatus::NX_ERROR) {
+  try {
+    ::NeXus::File nxHandle(getPropertyValue("Filename"), NXACC_READ);


The file is opened but not closed.

rboston628 · 2025-02-02T00:56:54Z

Framework/DataHandling/src/LoadILLIndirect2.cpp

-    g_log.debug() << "convertNexusToProperties: Error loading " << nexusfilename;
+
+  try {
+    ::NeXus::File nxfileID(nexusfilename, NXACC_READ);


The file is opened but not closed.

rboston628 · 2025-02-02T00:57:11Z

Framework/DataHandling/src/LoadILLDiffraction.cpp

-    NXclose(&nxHandle);
+  // get some information from the NeXus file
+  try {
+    ::NeXus::File filehandle(m_filename, NXACC_READ);


The file is opened but not closed.

rboston628 · 2025-02-02T01:13:14Z

Framework/DataHandling/src/LoadHelper.cpp

+        bool read_property = true;
+        if (nxinfo.type == NXnumtype::CHAR) {
+          read_property = (rank == 1);
+        } else if (isFloat) {


I think using isFloat obscures what is happening in this if/else block. Also, writing this as an if/else block obscures that it is actually a switch of nxinfo.type. Also, the integer cases aren't implemented in this if/else: should they be?

rboston628 · 2025-02-02T01:17:22Z

Framework/DataHandling/src/LoadHelper.cpp

+          // generate a name for the property
+          std::string property_name = (parent_name.empty() ? nxname : parent_name + "." + nxname);
+
+          if (nxinfo.type == NXnumtype::CHAR) {


As above, the use of if/else is obscuring that this is a switch over nxinfo.type. Maybe if there were also an isChar bool defined?

Also, can this not be one big switch on nxinfo.type, instead of two separate switches on the same value?

peterfpeterson added the Maintenance Unassigned issues to be addressed in the next maintenance period. label Jan 30, 2025

peterfpeterson added this to the Release 6.13 milestone Jan 30, 2025

peterfpeterson force-pushed the 38332_LoadILLDiffraction_refactor branch from f21ab79 to 07ccece Compare January 30, 2025 21:34

peterfpeterson marked this pull request as ready for review January 30, 2025 21:46

peterfpeterson force-pushed the 38332_LoadILLDiffraction_refactor branch from 07ccece to 50f0ee8 Compare January 31, 2025 14:01

rboston628 reviewed Jan 31, 2025

View reviewed changes

buildconfig/CMake/CppCheck_Suppressions.txt.in Show resolved Hide resolved

github-actions bot added the Has Conflicts Used by the bot to label pull requests that have conflicts label Jan 31, 2025

peterfpeterson added 2 commits January 31, 2025 13:59

Refactor LoadHelper to use NeXus::File

e1d8fab

Remove napi version and modify clients

aef68a0

peterfpeterson force-pushed the 38332_LoadILLDiffraction_refactor branch from 50f0ee8 to aef68a0 Compare January 31, 2025 18:59

peterfpeterson removed the Has Conflicts Used by the bot to label pull requests that have conflicts label Jan 31, 2025

rboston628 reviewed Feb 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor LoadHelper to use NeXus::File #38766

Refactor LoadHelper to use NeXus::File #38766

peterfpeterson commented Jan 30, 2025 •

edited

Loading

github-actions bot commented Jan 31, 2025

rboston628 left a comment

rboston628 Feb 2, 2025

rboston628 Feb 2, 2025

rboston628 Feb 2, 2025

rboston628 Feb 2, 2025

rboston628 Feb 2, 2025

rboston628 Feb 2, 2025

rboston628 Feb 2, 2025

rboston628 Feb 2, 2025

rboston628 Feb 2, 2025

rboston628 Feb 2, 2025

rboston628 Feb 2, 2025

Refactor LoadHelper to use NeXus::File #38766

Are you sure you want to change the base?

Refactor LoadHelper to use NeXus::File #38766

Conversation

peterfpeterson commented Jan 30, 2025 • edited Loading

To test:

Reviewer

Code Review

Functional Tests

Gatekeeper

github-actions bot commented Jan 31, 2025

rboston628 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterfpeterson commented Jan 30, 2025 •

edited

Loading