-
Notifications
You must be signed in to change notification settings - Fork 130
AlignAndFocusPowderSlim: Add GroupingWorkspace input and allow arbitrary grouping #40571
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
4a6a5fc to
acca369
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for arbitrary detector grouping to AlignAndFocusPowderSlim by introducing a GroupingWorkspace input property. This enables a many-to-many relationship between detector banks and output spectra, moving beyond the previous one-to-one bank-to-spectra mapping.
Key Changes:
- New
GroupingWorkspaceinput property allows users to specify custom detector groupings - Refactored event processing to support arbitrary grouping through a new
SpectraProcessingDatastructure - Modified
BankCalibrationandBankCalibrationFactoryto handle grouping intersections with bank detectors
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
Framework/Geometry/src/Instrument.cpp |
Added getDetectorIDsInBank method to retrieve detector IDs for a given bank |
Framework/Geometry/inc/MantidGeometry/Instrument.h |
Header declaration for new getDetectorIDsInBank method |
Framework/DataObjects/src/GroupingWorkspace.cpp |
Modified getGroupIDs to optionally exclude unset groups; changed return type of getDetectorIDsOfGroup |
Framework/DataObjects/inc/MantidDataObjects/GroupingWorkspace.h |
Updated method signatures for grouping workspace operations |
Framework/DataHandling/src/AlignAndFocusPowderSlim.cpp |
Major refactoring to support arbitrary grouping with validation and detector-to-spectrum mapping |
Framework/DataHandling/inc/MantidDataHandling/AlignAndFocusPowderSlim.h |
Added new helper methods and member variable for detector-to-spectrum mapping |
Framework/DataHandling/inc/MantidDataHandling/AlignAndFocusPowderSlim/SpectraProcessingData.h |
New structure for thread-safe accumulation of event counts using atomics |
Framework/DataHandling/src/AlignAndFocusPowderSlim/ProcessBankTask.cpp |
Refactored to handle both direct and arbitrary grouping modes |
Framework/DataHandling/src/AlignAndFocusPowderSlim/BankCalibration.cpp |
Changed to use std::set for detector IDs; simplified range determination logic |
Framework/DataHandling/inc/MantidDataHandling/AlignAndFocusPowderSlim/BankCalibration.h |
Updated API to work with sets and support multiple calibrations per bank |
Framework/DataHandling/test/AlignAndFocusPowderSlimTest.h |
Added comprehensive tests for 12-group and 3-group scenarios |
| Multiple test files | Updated to use std::set instead of std::vector for detector IDs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Framework/DataObjects/inc/MantidDataObjects/GroupingWorkspace.h
Outdated
Show resolved
Hide resolved
Framework/DataHandling/inc/MantidDataHandling/AlignAndFocusPowderSlim/BankCalibration.h
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Framework/DataHandling/src/AlignAndFocusPowderSlim/BankCalibration.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description of work
This allows you to provide a
GroupingWorkspacethat will be used to group the events by detector id to the correct output spectra. This means a many-to-many relationtionship between bank and output spectra. By default without aGroupingWorkspacethe events are put in a one-to-one bank to spectra mapping.The current implementation only works when not using a splitter workpsace but that ability will be added next.
Refs:
To test:
Simple usgae example
A easy way to test is to create a
GroupingWorkspacewithGenerateGroupingPowderwhich can then be compare to usingGroupDetectors.Using 10 degrees gives 12 output spectra
This is what the grouping looks like
Using 45 degrees gives only 3 output spectra
The grouping looks like
Grouping from calibartion file
If a calibration file but no GroupingWorkspace is provided then the grouping infortmation in the calibration file is used. e.g.
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: