mantidproject / mantid

Main repository for Mantid code
https://www.mantidproject.org
GNU General Public License v3.0
211 stars 124 forks source link

Change horizontal summing step for INTER 2D detector reduction #35684

Open rbauststfc opened 1 year ago

rbauststfc commented 1 year ago

As part of the work for #31069 we needed to implement a step that horizontally sums the pixel counts of the new INTER 2D detector before the rest of the reduction takes place. To do this we created an algorithm called ReflectometryISISSumBanks that wraps a call to the SmoothNeighbours algorithm, and we call this after the flood correction step from ReflectometryReductionOneAuto. Whether or not the summing step should be performed for the given instrument is determined by some logic in ReflectometryISISLoadAndProcess.

Some recent discussions have highlighted that there may be some fundamental issues with the science here that mean the approach needs to change. This is to do with the fact that the INTER detector's 8 banks of pixels are unlikely to have been perfectly aligned, so when summing across the horizontal rows you are actually combining counts that have been detected at slightly different vertical positions. This is considered an acceptable approximation in the short-term, particularly if the build turns out to be relatively accurate, however it negates a significant amount of the benefit of the extra width on the new detector. The scientists need to discuss and agree the science required to get an accurate reduction with the new detector and then we will need to consider implementation options for this.

In case it's relevant when we come to implementation, there are some existing limitations with the current horizontal summing implementation that it would be good to resolve:

1) The instrument must define a single rectangular detector panel to sum correctly. If there are multiple rectangular detectors then the SmoothNeighbours algorithm sums each individually, which is incorrect. The INTER 2D detector actually has four narrower panels in the middle, so they are currently unable to define their detector in a way that renders visually accurately in the instrument viewer, as this would require multiple rectangular detector panels. Additionally, the SmoothNeighbours algorithm performs a different type of summing (based on summing all pixels within a given radius rather than horizontally) if the IDF has a mix of rectangular and non-rectangular detectors. Due to a lack of validation in the code, this could currently happen silently if the IDF was changed.

2) The SmoothNeighbours algorithm produces a summed workspace that has spectra numbered sequentially beginning at 0. The ROI/ProcessingInstructions in the ISIS Reflectometry reduction are used to provide the spectra range for the reduction, however now the scientists will need to make sure they're using the spectra numbers that exist after the summing step, where previously they would have used the spectra as defined in the loaded file. These would always have been expected to change to some extent, as after summing we go from 2048 spectra to 256, however the change to the start of the numbering sequence relative to what was loaded is likely to create additional confusion. We can set sensible spectra range defaults in their parameter file (MultiDetectorStart and MultiDetectorStop values), and they can use the preview tab to select the summed spectra range from the slice viewer plot, however it would be good if any future solution minimised or eliminated the confusion here.

3) The ReflectometryISISSumBanks algorithm is written in Python but called from ReflectometryReductionOneAuto, which is a C++ algorithm. This seems to prevent unit tests being added to ReflectometryReductionOneAuto3Test because (for reasons I've not yet established) the Python API doesn't seem to be available to at least some of our C++ tests. For now the summing step is tested via the unit tests for ReflectometryISISLoadAndProcess, but this is not ideal. It would be good for this to be resolved in some way as part of the final solution. Note this should be possible to resolve by writing the desired unit tests in Python

github-actions[bot] commented 6 months ago

This issue has been automatically marked as stale because it has not had activity in 6 months. It will be closed in 7 days if no further activity occurs. Allowing issues to close as stale helps us filter out issues which can wait for future development time. All issues closed by stale bot act like normal issues; they can be searched for, commented on or reopened at any point. If you'd like a closed stale issue to be considered, feel free to either re-open the issue directly or contact a developer. To extend the lifetime of an issue please comment below, it helps us see that this is still affecting you and you want it fixed in the near-future. Extending the lifetime of an issue may cause the development team to prioritise it over other issues, which may be closed as stale instead.