Remove hdf4 support from NexusCpp #38954

peterfpeterson · 2025-02-24T15:20:02Z

This removes hdf4 from FrameworkNexus in its entirety.

LoadILLTOF2 was converted to use LegacyNexus (to support legacy hdf4 files) and LoadILLTOF3 is a copy that uses Nexus (hdf5). There is also a new set of tests for FileLoaderRegistry which helped figure out which loaders support which backend.

The change to LegacyNexus's make is because I discovered it was using set_target_properties incorrectly and the flag was making it in through find cmake find modules.

Refs #38332

To test:

This is mostly a refactor of code and additional unit tests were added to help verify it is working.

This does not require release notes because it is part of the nexus API consolidation which will have a more extensive write-up.

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.

github-actions · 2025-02-26T18:55:38Z

👋 Hi, @peterfpeterson,

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

github-actions · 2025-02-27T17:06:37Z

👋 Hi, @peterfpeterson,

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

rboston628

I'm not sure how to review LoadILLTOF3, and will need to do that tomorrow. Here are some comments on the rest

Framework/DataHandling/src/LoadNexus.cpp

rboston628 · 2025-02-27T22:04:14Z

Framework/DataHandling/test/LoadDetectorInfoTest.h

@@ -262,7 +262,7 @@ class LoadDetectorInfoTest : public CxxTest::TestSuite {

    makeTestWorkspace(SmallTestDatFile::NDETECTS, NBINS, m_InoutWS);
    loadDetInfo.setPropertyValue("Workspace", m_InoutWS);
-    loadDetInfo.setPropertyValue("DataFilename", "argus0026287.nxs");
+    loadDetInfo.setPropertyValue("DataFilename", "ARGUS00073601.nxs");


Why was this change necessary?

That was a fix from Ross. I assume that it was an issue with FileFinder / ArchiveSearch forgetting what the instrument is

But doesn't changing the file used for the test, effectively change what the test is testing? If it was necessary, it suggests some behavior changed

I'm not sure why. It is a Ross change

@rosswhitfield Was this change necessary, or is this file just faster to work with?

@MialLewis is this instrument a muon instrument?

This appears to be another hidden hdf4 based usage. For most everything else we've copied the code into the LegacyNexus portion of things. However, the way LoadDetectorInfo is used by muons isn't entirely clear. We need @MialLewis's advice

$ rg LoadDetectorInfo -l -t cpp -t py | grep -v LoadDetectorInfo scripts/Inelastic/Direct/RunDescriptor.py Testing/SystemTests/tests/framework/LoadLotsOfFiles.py Framework/WorkflowAlgorithms/src/DgsConvertToEnergyTransfer.cpp

Yeah I changed the test data from a HDF4 file to HDF5 just to get the test to pass but we should check if they need LoadDetectorInfo to support HDF4.

I also removed the HDF4 file from the StartAndEndTimeFromNexusFileExtractorTest because I didn't see any HDF4 algorithms using the extractStartTime or extractEndTime functions.

ARGUS is indeed a Muon instrument - I've passed this question on to a Muon scientist to see if they actually use this algorithm.

Documenting the discussion that happened on slack. This particular issue (ARGUS) is being resolved by deleting it for now. If we need it later, the code will be resurrected.

Framework/DataHandling/test/LoadILLTOF2Test.h

Framework/DataHandling/test/StartAndEndTimeFromNexusFileExtractorTest.h

Framework/API/test/FileLoaderRegistryTest.h

Framework/DataHandling/src/LoadILLTOF2.cpp

github-actions · 2025-03-03T18:23:34Z

👋 Hi, @peterfpeterson,

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

This required copying a bunch of code from LoadHelper over into the LoadILLTOF2 cpp file.

By not suggesting a loader, this uses generic load which correctly selects the correct version of LoadILLTOF.

rboston628

I am accepting the ARGUS issue as resolved. That was my only hesitation. All looks good.

peterfpeterson added Maintenance Unassigned issues to be addressed in the next maintenance period. Technical Debt Marks a piece of work to address technical debt introduced to solve a problem quickly labels Feb 24, 2025

peterfpeterson added this to the Release 6.13 milestone Feb 24, 2025

peterfpeterson force-pushed the 38332_disable_hdf4 branch 5 times, most recently from 2cd36c9 to 344015c Compare February 26, 2025 17:01

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

peterfpeterson force-pushed the 38332_disable_hdf4 branch 2 times, most recently from 14777bf to 971c9cc Compare February 26, 2025 22:06

github-actions bot added Has Conflicts Used by the bot to label pull requests that have conflicts and removed Has Conflicts Used by the bot to label pull requests that have conflicts labels Feb 27, 2025

peterfpeterson force-pushed the 38332_disable_hdf4 branch from 971c9cc to 4e28a7b Compare February 27, 2025 19:21

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

peterfpeterson marked this pull request as ready for review February 27, 2025 20:39

peterfpeterson force-pushed the 38332_disable_hdf4 branch from 4e28a7b to 4717a8b Compare February 27, 2025 21:23

rboston628 reviewed Feb 27, 2025

View reviewed changes

sf1919 assigned rboston628 Feb 28, 2025

peterfpeterson force-pushed the 38332_disable_hdf4 branch 2 times, most recently from 9d622ee to 204b4bc Compare March 3, 2025 15:52

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

peterfpeterson force-pushed the 38332_disable_hdf4 branch from 204b4bc to 292daf3 Compare March 3, 2025 19:07

peterfpeterson and others added 4 commits March 3, 2025 18:05

Remove hdf4 support from NexusCpp

237f693

Fix napi.h in LegacyNexus namespace

661ab96

Update or remove test files

d52b919

Move LoadNexus to use LegacyNexus

e1c7169

peterfpeterson and others added 11 commits March 3, 2025 18:05

Move to LegacyNexusDescrpitor because some IN5 files are hdf4

94b9044

Add broken test for hdf4 files

6d570af

Adding LoadILLTOF3 based on HDF5

cd0d711

Move LegacyNexusClasses into LegacyNexus

60d6206

Get LoadILLTOF2 working with LegacyNexus

040f7f7

This required copying a bunch of code from LoadHelper over into the LoadILLTOF2 cpp file.

Minor code cleanup

b13aa04

Copy over EOD_ENTRY from FrameworkNexus for clearer code

95bc19c

Modify assert to give clear error message

f830a7b

Minor code cleanup

0df1439

Register the loader correctly

9fe2ca2

Remove HDF4 from the nexus tests again

7d9f09d

peterfpeterson force-pushed the 38332_disable_hdf4 branch from 292daf3 to 7d9f09d Compare March 3, 2025 23:16

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

Do not suggest a loader

476596f

By not suggesting a loader, this uses generic load which correctly selects the correct version of LoadILLTOF.

rboston628 approved these changes Mar 4, 2025

View reviewed changes

KedoKudo merged commit 669dd76 into mantidproject:main Mar 4, 2025
10 checks passed

peterfpeterson mentioned this pull request Mar 4, 2025

Remove hdf4 support from NexusCpp - ornl-next #39009

Merged

peterfpeterson deleted the 38332_disable_hdf4 branch March 4, 2025 20:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove hdf4 support from NexusCpp #38954

Remove hdf4 support from NexusCpp #38954

peterfpeterson commented Feb 24, 2025 •

edited

Loading

github-actions bot commented Feb 26, 2025

github-actions bot commented Feb 27, 2025

rboston628 left a comment

rboston628 Feb 27, 2025

peterfpeterson Feb 28, 2025

rboston628 Feb 28, 2025

peterfpeterson Feb 28, 2025

rboston628 Feb 28, 2025

rboston628 Feb 28, 2025

peterfpeterson Feb 28, 2025

rosswhitfield Mar 2, 2025

MialLewis Mar 3, 2025

peterfpeterson Mar 3, 2025

github-actions bot commented Mar 3, 2025

rboston628 left a comment

Remove hdf4 support from NexusCpp #38954

Remove hdf4 support from NexusCpp #38954

Conversation

peterfpeterson commented Feb 24, 2025 • edited Loading

To test:

Reviewer

Code Review

Functional Tests

Gatekeeper

github-actions bot commented Feb 26, 2025

github-actions bot commented Feb 27, 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

github-actions bot commented Mar 3, 2025

rboston628 left a comment

Choose a reason for hiding this comment

peterfpeterson commented Feb 24, 2025 •

edited

Loading