Migrate LoadNexusProcessed for lazy file descriptor
#40596
Conversation
rboston628
added
the
Technical Debt
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis pull request migrates Nexus file loaders to use lazy-loading descriptors. LoadNexusProcessed and LoadNexusProcessed2 are updated to inherit from IFileLoader instead of NexusFileLoader, with corresponding changes to their confidence method signatures and registration macros. The execLoader() method is renamed to exec(). A new classTypeExists() public method is added to the NexusFile class to query descriptor class types, and existing instrument presence checks are updated to use this new API. Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Framework/DataHandling/inc/MantidDataHandling/LoadNexusProcessed.h (1)
15-15: Remove the unusedNexusFileLoader.hinclude.Since
LoadNexusProcessednow inherits fromIFileLoader<NexusDescriptorLazy>instead ofNexusFileLoader, the include on line 15 is unused and can be safely removed.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Framework/DataHandling/inc/MantidDataHandling/LoadNexusProcessed.hFramework/DataHandling/inc/MantidDataHandling/LoadNexusProcessed2.hFramework/DataHandling/src/LoadNexusProcessed.cppFramework/DataHandling/src/LoadNexusProcessed2.cppFramework/Nexus/inc/MantidNexus/NexusFile.h
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,cc,cxx,c++,h,hpp,hxx}
📄 CodeRabbit inference engine (AGENTS.md)
Run clang-tidy for code linting using the configuration in .clang-tidy
Files:
Framework/Nexus/inc/MantidNexus/NexusFile.hFramework/DataHandling/inc/MantidDataHandling/LoadNexusProcessed2.hFramework/DataHandling/src/LoadNexusProcessed.cppFramework/DataHandling/src/LoadNexusProcessed2.cppFramework/DataHandling/inc/MantidDataHandling/LoadNexusProcessed.h
🧠 Learnings (3)
📚 Learning: 2026-01-07T17:35:58.215Z
Learnt from: rboston628
Repo: mantidproject/mantid PR: 40570
File: Framework/Nexus/src/NexusDescriptorLazy.cpp:129-130
Timestamp: 2026-01-07T17:35:58.215Z
Learning: In Framework/Nexus/src/NexusDescriptorLazy.cpp, the pattern of using std::string with HDF5's H5Gget_objname_by_idx is intentional: a string of size name_len is created, which has an underlying buffer of name_len + 1 bytes, allowing safe writes of name_len + 1 characters including the null terminator.
Applied to files:
Framework/DataHandling/inc/MantidDataHandling/LoadNexusProcessed2.hFramework/DataHandling/inc/MantidDataHandling/LoadNexusProcessed.h
📚 Learning: 2025-10-22T13:08:14.586Z
Learnt from: GuiMacielPereira
Repo: mantidproject/mantid PR: 40194
File: Framework/Algorithms/src/CreateDetectorTable.cpp:64-68
Timestamp: 2025-10-22T13:08:14.586Z
Learning: In Mantid Framework, MatrixWorkspace::getInstrument() (inherited from ExperimentInfo) throws an exception if no instrument is found rather than returning nullptr. The method dereferences the internal instrument pointer, so null-checking the return value of getInstrument() is unnecessary. Only check the objects returned by methods called on the instrument (e.g., getSample()).
Applied to files:
Framework/DataHandling/src/LoadNexusProcessed.cpp
📚 Learning: 2025-11-07T16:15:26.454Z
Learnt from: rboston628
Repo: mantidproject/mantid PR: 40287
File: Framework/Algorithms/src/FFTSmooth2.cpp:79-87
Timestamp: 2025-11-07T16:15:26.454Z
Learning: In Mantid histogram data, the X-axis represents bin edges, so there are always N+1 X values for N Y values. The final bin may legitimately have a different (smaller) width when the data is linearly binned with start/stop and bin width parameters.
Applied to files:
Framework/DataHandling/inc/MantidDataHandling/LoadNexusProcessed.h
🧬 Code graph analysis (5)
Framework/Nexus/inc/MantidNexus/NexusFile.h (1)
Framework/Nexus/src/NexusDescriptor.cpp (2)
classTypeExists(204-204)classTypeExists(204-204)
Framework/DataHandling/inc/MantidDataHandling/LoadNexusProcessed2.h (3)
Framework/DataHandling/src/LoadNexusProcessed.cpp (2)
confidence(197-202)confidence(197-197)Framework/DataHandling/src/LoadNexusProcessed2.cpp (2)
confidence(238-243)confidence(238-238)Framework/Nexus/src/NexusDescriptorLazy.cpp (1)
NexusDescriptorLazy(56-58)
Framework/DataHandling/src/LoadNexusProcessed.cpp (4)
Framework/DataHandling/src/LoadMcStasNexus.cpp (3)
DECLARE_NEXUS_LAZY_FILELOADER_ALGORITHM(22-26)version(29-29)version(29-29)Framework/DataHandling/src/LoadNexusProcessed2.cpp (4)
confidence(238-243)confidence(238-238)version(73-73)version(73-73)Framework/DataHandling/src/LoadEMU.cpp (8)
confidence(1186-1200)confidence(1186-1186)confidence(1281-1303)confidence(1281-1281)version(1171-1171)version(1171-1171)version(1263-1263)version(1263-1263)Framework/DataHandling/src/LoadPLN.cpp (4)
confidence(829-843)confidence(829-829)version(814-814)version(814-814)
Framework/DataHandling/src/LoadNexusProcessed2.cpp (3)
Framework/DataHandling/src/LoadILLSALSA.cpp (1)
DECLARE_NEXUS_LAZY_FILELOADER_ALGORITHM(25-27)Framework/DataHandling/src/LoadNexusProcessed.cpp (2)
confidence(197-202)confidence(197-197)Framework/API/src/FileLoaderRegistry.cpp (2)
descriptor(25-25)descriptor(25-25)
Framework/DataHandling/inc/MantidDataHandling/LoadNexusProcessed.h (3)
Framework/DataHandling/inc/MantidDataHandling/LoadNexusProcessed2.h (2)
API(19-22)Mantid(18-69)Framework/Nexus/src/NexusDescriptorLazy.cpp (1)
NexusDescriptorLazy(56-58)Framework/DataHandling/src/LoadNexusProcessed.cpp (4)
confidence(197-202)confidence(197-197)exec(377-545)exec(377-377)
🪛 Cppcheck (2.19.0)
Framework/DataHandling/src/LoadNexusProcessed2.cpp
[error] 27-27: There is an unknown macro here somewhere. Configuration is required. If DECLARE_NEXUS_LAZY_FILELOADER_ALGORITHM is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (13)
Framework/Nexus/inc/MantidNexus/NexusFile.h (1)
571-576: LGTM!The new
classTypeExistsmethod provides a clean public API for querying class types in the Nexus file by delegating to the internal descriptor. This enables theLoadNexusProcessedalgorithm to check for instrument presence without directly depending onNexusDescriptor.Framework/DataHandling/src/LoadNexusProcessed.cpp (4)
55-55: LGTM!The registration macro correctly updated to
DECLARE_NEXUS_LAZY_FILELOADER_ALGORITHMto register the loader with the lazy descriptor type, consistent with the migration pattern used in other loaders likeLoadMcStasNexusandLoadILLSALSA.
197-202: LGTM!The
confidencemethod signature correctly updated to useNexusDescriptorLazy. The logic remains unchanged, and theisEntrymethod is available on the lazy descriptor type.
377-377: LGTM!Method correctly renamed from
execLoader()toexec()to match theIFileLoader<NexusDescriptorLazy>interface override point.
1939-1944: LGTM!The instrument presence check correctly updated to use the new
classTypeExistsAPI. The conditionversion() < 2 || m_nexusFile->classTypeExists("NXinstrument")maintains the original semantics: show warnings for v1 loaders or when an NXinstrument entry exists (indicating the file was expected to have instrument data).Framework/DataHandling/src/LoadNexusProcessed2.cpp (2)
27-27: LGTM!Registration macro correctly updated to
DECLARE_NEXUS_LAZY_FILELOADER_ALGORITHMto align with the parent class migration. The cppcheck warning about an unknown macro is a false positive—this macro is defined inMantidAPI/RegisterFileLoader.h.
238-243: LGTM!The
confidencemethod signature correctly updated to useNexusDescriptorLazy. The implementation properly calls the parent class'sconfidence(descriptor) + 1to indicate this v2 loader is incrementally more suitable than v1 when the file matches.Framework/DataHandling/inc/MantidDataHandling/LoadNexusProcessed2.h (2)
14-14: LGTM!Include correctly updated to
NexusDescriptorLazy.hto support the new descriptor type used in theconfidencemethod signature.
44-44: LGTM!The
confidencemethod declaration correctly updated to useNexusDescriptorLazy, matching both the parent class signature and the implementation in the corresponding.cppfile.Framework/DataHandling/inc/MantidDataHandling/LoadNexusProcessed.h (4)
21-21: LGTM!Include correctly added for
NexusDescriptorLazy.hto support the new descriptor type.
46-46: LGTM!Base class correctly changed from
API::NexusFileLoadertoAPI::IFileLoader<Mantid::Nexus::NexusDescriptorLazy>. This is the core change enabling the lazy file descriptor migration.
70-70: LGTM!The
confidencemethod declaration correctly updated to useNexusDescriptorLazy, matching theIFileLoaderinterface requirements.
84-84: LGTM!Method correctly renamed from
execLoader()toexec()to match theIFileLoaderinterface override point.
Description of work
In PR #40570, a new file descriptor,
NexusDescriptorLazy, was created for shallow and lazy checking of files as part of theLoadalgorithm's confidence check.Many algorithms could be migrated mechanically, as in PR #40583.
This PR is migrating the algorithm
LoadNexusProcessedalgorithm. Its confidence check is trivially portable. However, this inherited fromNexusFileLoader, which requires aNexusDescriptorfor the confidence check. One location in the code relied on the inheritedNexusDescriptor, so that it could not be trivially migrated to use the lazy version.This adds a new method to
Nexus::Filewhich passes through to theNexusDescriptorto call itsclassNameExistsmethod, which is what is actually needed inside this load algo. With that, theNexusDecriptoris no longer needed and this can be set to inherit fromIFileLoader<NexusDescriptorLazy>.Related to Issue #37164
EWM 14485
To test:
This is a refactor, so ensure all current tests works. A test of the confidence calculation already existed, and a new one was added to make sure
Loadcan still correctly find this loader.Especially look at the
LoadLotsOfFilessystem test.Tests in PR #40570 verified that
Loadis able to find load algorithms with a lazy descriptor.Reviewer
Your comments will be used as part of the gatekeeper process. Comment clearly on what you have checked and tested during your review. Provide an audit trail for any changes requested.
As per the review guidelines:
mantid-developersormantid-contributorsteams, add a review commentrerun cito authorize/rerun the CIGatekeeper
As per the gatekeeping guidelines: