Allow for LoadEventNexus to properly load only a single bank on MANDI instruments

[rboston628]

The Instrument object defines a detector cache. Though the logic of the cache should follow that of an ordered map, at some point this cache was implemented as vector. This implementation had tradeoffs.

On the positive side, detectors could be more quickly inserted into the cache.

On the negative side, searching for an element becomes $O(N)$. This was partly fixed by keeping the vector sorted and using a binary search algorithm on it, which kept search times at $O(\log N)$.

However, for a vector, the erase method is always $O(N)$. The erase method was called with the Instrument::removeDetector() method.

One particular hot path for this method occurred in LoadEventNexus::deleteBanks(), which called removeDetector() for (almost) every single detector in the instrument.

The error was discovered as the cause of a user-reported error, where calling LoadEventNexus for a single bank for data in the MANDI instrument was apparently freezing. It was in fact taking hours to loop through every pixel, find it, and erase it from the detector cache, taking at least $O(N^2)$ operations.

A previous attempt in PR #41176 tried to replace the cache with an ordered map. This did improve search speeds; however, some of the vector operations occur on hot paths, leading to longer run times in some tests.

Moreover, even when the time-complexity issue was fixed, Instrument::deleteBanks() still cause an unrelated error arising from a mismatch between the instrument's cache size, and the instrument size recorded in the DetectorInfo object. This latter error was proven to exist as far back as Mantid 6.6.

Because of the error, previous work in PR #41183 removed the feature to delete unused banks in MANDI and TOPAZ instruments (the section of code causing the freeze).

This PR has fixed the error in two ways.

1. A new set of methods removeDetectorIncomplete and removeDetectorFinalize, analogous to the markAsMonitor methods, has been added for quickly deleting an large number of detectors from the cache. This stores up the detectors to delete, then uses remove_if to remove them all at the end, reducing time complexity. LoadEventNexus will no longer appear to freeze. (NOTE: I ended up not using these, see below)
2. A new instrument is created, cloned from the full instrument, but with only the desired bank and its detectors added into it. This was necessary, to avoid modifying the original instrument in the Instrument data service.
3. A new method was added to force the ComponentInfo and DetectorInfo objects to update after the instrument was reset. This fixed the issue of size mismatch.

Because both errors were addressed, the delete-banks feature was reinstated.

Refs:

• EWM 15489
• Remove the delete banks feature from LoadEventNexus #41183
• Add finalized flag to Instrument to ensure the cache is finalized #41187
• Change Instrument detector cache for faster lookup #41176
• Remove deleting each individual pixel in LoadEventNexus #41063
• Change Instrument detector cache for cleaner code #41205

To test:

Build and run the following in workbench:

outws = LoadEventNexus("MANDI_13064", BankName="bank26")

This should run in only a few seconds. There should be no errors produced during LoadEventNexus.

Right-click the workspace to check the instrument. It should be named "MANDI_clone_bank26". It should contain only bank26. There are no other banks in the instrument view, and no other banks in the instrument listing.

Re-run the script with "bank27", and with no bank name specified (i.e. full instrument), and ensure behavior is as expected.

Please carefully explore the instrument and the workspace data to make sure that this behaves the way we expect a single-bank-loaded workspace to behave.

---

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:

• Is the code of an acceptable quality? (Code standards/GUI standards)
• Has a thorough functional test been performed? Do the changes handle unexpected input/situations?
• Are appropriately scoped unit and/or system tests provided?
• Do the release notes conform to the guidelines and describe the changes appropriately?
• Has the relevant (user and developer) documentation been added/updated?
• If the PR author isn’t in the mantid-developers or mantid-contributors teams, add a review comment rerun ci to authorize/rerun the CI

Gatekeeper

As per the gatekeeping guidelines:

• Has a thorough first line review been conducted, including functional testing?
• At a high-level, is the code quality sufficient?
• Are the base, milestone and labels correct?

[Copilot]

Pull request overview

This PR restores and accelerates LoadEventNexus “delete unused banks” behavior (notably for MANDI/TOPAZ single-bank loads) by introducing bulk-removal support for the Instrument detector cache and forcing regeneration of ComponentInfo/DetectorInfo after instrument resets to avoid size-mismatch errors.

Changes:

• Add Instrument::removeDetectorIncomplete/removeDetectorFinalize to bulk-remove many detectors from the cache efficiently.
• Re-enable/trigger bank deletion in LoadEventNexus via an instrument parameter (remove-unused-banks) and reset the workspace instrument after deletion.
• Force fresh beamline wrapper creation for empty ParameterMap instrumentation to prevent stale ComponentInfo/DetectorInfo reuse.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
Framework/Geometry/src/Instrument/ParameterMap.cpp	Uses a “force new beamline” path when the parameter map is empty to avoid stale wrapper reuse.
Framework/Geometry/src/Instrument/InstrumentVisitor.cpp	Minor control-flow braces adjustment.
Framework/Geometry/src/Instrument.cpp	Refactors detector-cache access and adds bulk-removal + cache helper methods.
Framework/Geometry/inc/MantidGeometry/Instrument.h	Introduces DetectorCacheEntry/DetectorCache types and new bulk-removal APIs.
Framework/DataHandling/src/LoadEventNexus.cpp	Reintroduces bank deletion path and switches to bulk detector-cache removal; resets instrument after mutation.
Framework/API/src/ExperimentInfo.cpp	Improves mismatch error message ordering and adds braces for clarity.
docs/source/release/v6.16.0/Framework/Algorithms/Bugfixes/41215.rst	Adds release note for the restored/fixed single-bank loading behavior.
docs/source/release/v6.16.0/Framework/Algorithms/Bugfixes/41183.rst	Removes the prior release note stating the feature was removed.
buildconfig/CMake/CppCheck_Suppressions.txt.in	Updates a suppression line number after source changes.

Comments suppressed due to low confidence (1)

Framework/Geometry/src/Instrument.cpp:795

• The exception message here uses "parameterized" while most other messages in this file use "parametrized". Consider using a consistent spelling across Instrument error messages (and update the other new occurrences in this patch) to keep logs/searchability consistent.

void Instrument::removeDetector(IDetector *det) {
  if (m_map)
    throw std::runtime_error("Instrument::removeDetector() called on a "
                             "parameterized Instrument object.");
  if (!isFinalized())

[rboston628]

Note on TOPAZ

The feature to remove unused banks was originally created for TOPAZ, over 13 years ago. See commit 406d04f.

At a slightly later date, it was guarded to only run on instruments with the "remove-unused-banks" parameter flag, which was then limited to TOPAZ. See commits

• 8de375e
• cfc6005

The parameter was then deliberately removed, and then added back in set to 0:

• 9395917
• 05660b4

So for about 12 years, this feature intended for TOPAZ has not actually been running on TOPAZ. This can be fixed by changing this parameter back.

This has been set on MANDI for 12 years:

• 719b66c

It is unclear why no one has noticed the error for MANDI in this time.

[peterfpeterson]

Does this affect the singleton in the InstrumentDataService? I think the test is to load the same file into two different workspaces with different single banks selected.

[rboston628]

You can run with bank26, bank27, and with no bank given, and all three will have different instruments

[peterfpeterson]

It looks like this test is answering the question I had