mantid icon indicating copy to clipboard operation
mantid copied to clipboard

Add ROI Detector ID field to Experiment lookup table

Open robertapplin opened this issue 2 years ago • 1 comments

Description of work. This PR adds the ROI Detector ID column to the Experiment Lookup table. It also passes this property through to the relevant algorithm when processing is performed.

To test:

  1. Open the ISIS Reflectometry GUI
  2. Type INTER45455_inst in the runs table and an angle of 0.5
  3. Go to the experiment settings and create a setting for angle 0.5, and change the ROI Detector IDs column to 2001
  4. Go back to the runs table and process all
  5. Open the history of the output workspace and check that 2001 has been passed into the algorithm

Part of #34110


Reviewer

Please comment on the following (full description):

Code Review
  • Is the code of an acceptable quality?
  • Does the code conform to the coding standards?
  • Are the unit tests small and test the class in isolation?
  • If there is GUI work does it follow the GUI standards?
  • If there are changes in the release notes then do they describe the changes appropriately?
  • Are the release notes saved in a separate file, using Issue or PR number for file name and in the correct location?
Functional Tests
  • Do changes function as described? Add comments below that describe the tests performed?
  • Do the changes handle unexpected situations, e.g. bad input?
  • Has the relevant (user and developer) documentation been added/updated?

Does everything look good? Mark the review as Approve. A member of @mantidproject/gatekeepers will take care of it.

robertapplin avatar Sep 05 '22 10:09 robertapplin

@robertapplin Just spotted a problem testing this with SURF e.g. run 136500, angle 0.5. The reduction now fails because it's attempting to run sum banks. This data is from a run using the point-detector, but it looks like the IDF also has a 2D detector defined! So it looks like we may need to expand our checks. However I suspect we broke this in the previous PR to add the summing to the algorithm? Do you have a nightly build handy in which you can check that?

gemmaguest avatar Sep 09 '22 10:09 gemmaguest

Does this need a release note?

DannyHindson avatar Oct 19 '22 10:10 DannyHindson

Does this need a release note?

Technically yes, although this is tied in with the large ROI project and doesn't make so much sense as a standalone change, so I'll add it in a future PR at the same time as that gets done.

gemmaguest avatar Oct 19 '22 11:10 gemmaguest