Remove deleting each individual pixel in LoadEventNexus#41063
Remove deleting each individual pixel in LoadEventNexus#41063rboston628 wants to merge 2 commits intomainfrom
LoadEventNexus#41063Conversation
rboston628
changed the title
LoadEventNexus
There was a problem hiding this comment.
Pull request overview
This PR aims to address a performance regression in LoadEventNexus::deleteBanks() when loading a single bank (e.g., for MANDI) by avoiding per-pixel deletion and instead removing unused banks more directly.
Changes:
- Replaced the nested loop that matched bank names with a simpler
std::any_ofcheck. - Removed the per-pixel detector deletion loop and now only removes the bank component from the instrument tree.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if (!keep) { | ||
| const size_t parentIndex = componentInfo.indexOfAny(det_name); | ||
| const auto children = componentInfo.children(parentIndex); | ||
| for (const auto &colIndex : children) { | ||
| const auto grandchildren = componentInfo.children(colIndex); | ||
|
|
||
| for (const auto &rowIndex : grandchildren) { | ||
| auto *d = dynamic_cast<Detector *>(const_cast<IComponent *>(componentInfo.componentID(rowIndex))); | ||
| if (d) | ||
| inst->removeDetector(d); | ||
| } | ||
| } | ||
| auto *comp = dynamic_cast<IComponent *>(det.get()); | ||
| inst->remove(comp); |
There was a problem hiding this comment.
inst->remove(comp) calls CompAssembly::remove, which deletes the RectangularDetector (and therefore all pixel Detector children), but it does not update Instrument's m_detectorCache (which holds non-owning pointers via NoDeleting). This can leave the instrument with a detector cache containing dangling pointers, leading to crashes when code later calls getDetector(...)/detectorInfo()/etc. Consider rebuilding the detector cache after bank removal or providing a bulk-removal path that keeps the cache consistent with the component tree.
| } | ||
| } | ||
| auto *comp = dynamic_cast<IComponent *>(det.get()); | ||
| inst->remove(comp); |
There was a problem hiding this comment.
inst->remove(comp) will throw unless comp is a direct child of inst (it uses CompAssembly::remove which only searches m_children). But detList is populated from sub-assemblies (see the j/k loops above), so for instruments where rectangular banks are nested (the comment mentions PG3), this call can raise a runtime_error instead of removing the bank. Remove via the bank's actual parent assembly (e.g., det->getParent()) or track the owning assembly when building detList.
| inst->remove(comp); | |
| if (!comp) | |
| continue; | |
| // Remove via the bank's actual parent assembly to avoid exceptions when | |
| // the bank is not a direct child of the top-level instrument (e.g. PG3). | |
| auto parent = det->getParent(); | |
| auto parentAssembly = std::dynamic_pointer_cast<ICompAssembly>(parent); | |
| if (parentAssembly) { | |
| parentAssembly->remove(comp); | |
| } else { | |
| // Fallback for instruments where the detector is a direct child of inst | |
| inst->remove(comp); | |
| } |
| for (auto &det : detList) { | ||
| bool keep = false; | ||
| std::string det_name = det->getName(); | ||
| for (const auto &bankName : bankNames) { | ||
| size_t pos = bankName.find("_events"); | ||
| if (det_name == bankName.substr(0, pos)) | ||
| keep = true; | ||
| if (keep) | ||
| break; | ||
| } | ||
| std::string det_name = det->getName() + "_events"; | ||
| bool keep = | ||
| std::any_of(bankNames.cbegin(), bankNames.cend(), [&det_name](std::string const &s) { return s == det_name; }); | ||
| if (!keep) { | ||
| const size_t parentIndex = componentInfo.indexOfAny(det_name); | ||
| const auto children = componentInfo.children(parentIndex); | ||
| for (const auto &colIndex : children) { | ||
| const auto grandchildren = componentInfo.children(colIndex); | ||
|
|
||
| for (const auto &rowIndex : grandchildren) { | ||
| auto *d = dynamic_cast<Detector *>(const_cast<IComponent *>(componentInfo.componentID(rowIndex))); | ||
| if (d) | ||
| inst->removeDetector(d); | ||
| } | ||
| } | ||
| auto *comp = dynamic_cast<IComponent *>(det.get()); | ||
| inst->remove(comp); | ||
| } |
There was a problem hiding this comment.
This change significantly alters how unused banks are removed when remove-unused-banks is enabled, but there does not appear to be any unit/system test coverage that exercises deleteBanks() (existing LoadEventNexusTest single-bank tests do not enable the remove-unused-banks instrument parameter). Adding a regression test for single-bank loading with remove-unused-banks=1 would help prevent future performance/consistency regressions in this area (e.g., ensuring the instrument tree and detector cache remain usable after bank removal).
|
This appears to resolve the issue |
Description of work
In PR #40884 , instances using Instrument'sgetChildren()array were replaced with the methods inComponentInfo. However, this PR was apparently responsible for a performance regression discovered when trying to load a single bank for theMANDIinstrument.I have checked, and this is not the source of the regression. So I'm not sure how this problem hadn't been noticed before.
The replacement was inside the
deleteBanks()method ofLoadEventNexus. If only a single bank is loaded, the code cycles through all of the pixels in the entire instrument which are not in the chosen bank, and deletes them. This must have been a lot faster using the older Instrument methods, but it now takes such a long time that it users think the system has frozen. In this particular case, the file being loaded had very few events, so that it paradoxically took less time to load the entire file, than it takes to load a single bank.It is likely that the loop to remove all of the individual pixels isn't necessary. Just removing the banks should be sufficient.
EWM 15489
To test:
Build and run the following in workbench:
Check the instrument; everything should be empty except for bank26.
There happen to be very few events in this run, but to ensure thee are events, look at spectrum
1710492.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:
mantid-developersormantid-contributorsteams, add a review commentrerun cito authorize/rerun the CIGatekeeper
As per the gatekeeping guidelines: