Add CorrectionType property to SpecularReflectionPositionCorrect

[gemmaguest]

This PR adds a new property, CorrectionType, to SpecularReflectionPositionCorrect. This specifies whether detector positions should be corrected by a vertical shift (default) or by a rotation around the sample position. The new property is also exposed in ReflectometryReductionOneAuto and the GUI.

To test:
The following script demonstrates the usage of the new property. As per the expected output, the detector positions are slightly different depending on whether the correction was a vertical shift or a rotation.

polref = Load(Filename=r'POLREF00004699.raw', PeriodList=1)
polref_vert = SpecularReflectionPositionCorrect(polref[0], TwoTheta = 2*0.49, DetectorComponentName='lineardetector')
polref_rot = SpecularReflectionPositionCorrect(polref[0], TwoTheta = 2*0.49, DetectorComponentName='lineardetector', CorrectionType='RotateAroundSample')
print 'Original position: ' + str(polref[0].getInstrument().getComponentByName('lineardetector').getPos())
print 'Vertical shift:    ' + str(polref_vert.getInstrument().getComponentByName('lineardetector').getPos())
print 'Rotated:           ' + str(polref_rot.getInstrument().getComponentByName('lineardetector').getPos())

Expected output:

Original position: [26,0,0]
Vertical shift:    [26,0,0.0513177]
Rotated:           [25.9996,0,0.0513102]

Fixes #18722 .

Added to the release notes.

---

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? Is it well structured with small focussed classes/methods/functions?
• Are there unit/system tests in place? Are the unit tests small and test the a class in isolation?
• If there are changes in the release notes then do they describe the changes appropriately?

Functional Tests

• Do changes function as described? Add comments below that describe the tests performed?

• How do the changes handle unexpected situations, e.g. bad input?

• Has the relevant documentation been added/updated?

• Is user-facing documentation written in a user-friendly manner?

• Has developer documentation been updated if required?

• Does everything look good? Comment with the ship it emoji but don't merge. A member of @mantidproject/gatekeepers will take care of it.

[raquelalvarezbanos]

Functionality looks good. I've also tried with other reflectometry instruments:

inter = Load(Filename='INTER13460')
inter_vert = SpecularReflectionPositionCorrect(inter, TwoTheta = 45, DetectorComponentName='point-detector')
inter_rot = SpecularReflectionPositionCorrect(inter, TwoTheta = 45, DetectorComponentName='point-detector', CorrectionType='RotateAroundSample')
inter_sample = inter.getInstrument().getComponentByName('some-surface-holder')
print 'Original position: ' + str(inter.getInstrument().getComponentByName('point-detector').getPos()-inter_sample.getPos())
print 'Vertical shift:    ' + str(inter_vert.getInstrument().getComponentByName('point-detector').getPos()-inter_sample.getPos())
print 'Rotated:           ' + str(inter_rot.getInstrument().getComponentByName('point-detector').getPos()-inter_sample.getPos())

offspec = Load(Filename='OFFSPEC4622')
offspec_vert = SpecularReflectionPositionCorrect(offspec, TwoTheta = 45, DetectorComponentName='WLSFDetector')
offspec_rot = SpecularReflectionPositionCorrect(offspec, TwoTheta = 45, DetectorComponentName='WLSFDetector', CorrectionType='RotateAroundSample')
offspec_sample = offspec.getInstrument().getComponentByName('some-surface-holder')
print 'Original position: ' + str(offspec.getInstrument().getComponentByName('WLSFDetector').getPos()-offspec_sample.getPos())
print 'Vertical shift:    ' + str(offspec_vert.getInstrument().getComponentByName('WLSFDetector').getPos()-offspec_sample.getPos())
print 'Rotated:           ' + str(offspec_rot.getInstrument().getComponentByName('WLSFDetector').getPos()-offspec_sample.getPos())

crisp = Load(Filename='CSP78173.raw', PeriodList=1)
crisp_vert = SpecularReflectionPositionCorrect(crisp[0], TwoTheta = 45, DetectorComponentName='point-detector')
crisp_rot = SpecularReflectionPositionCorrect(crisp[0], TwoTheta = 45, DetectorComponentName='point-detector', CorrectionType='RotateAroundSample')
crisp_sample = crisp[0].getInstrument().getComponentByName('some-surface-holder')
print 'Original position: ' + str(crisp[0].getInstrument().getComponentByName('point-detector').getPos()-crisp_sample.getPos())
print 'Vertical shift:    ' + str(crisp_vert.getInstrument().getComponentByName('point-detector').getPos()-crisp_sample.getPos())
print 'Rotated:           ' + str(crisp_rot.getInstrument().getComponentByName('point-detector').getPos()-crisp_sample.getPos())

The output is:

Original position: [0,0.065984,2.663]
Vertical shift:    [0,2.663,2.663]
Rotated:           [0,1.8836,1.8836]

Original position: [0,0,3.62]
Vertical shift:    [0,3.62,3.62]
Rotated:           [0,2.55973,2.55973]

Original position: [0,0,1.863]
Vertical shift:    [0,1.863,1.863]
Rotated:           [0,1.31734,1.31734]

[raquelalvarezbanos]

I just have a minor comment about the new property name: while I think CorrectionType is a good name to use in SpecularReflectionPositionCorrect, I think it could cause confusion to scientists when they see it in ReflectometryReductionOneAuto. The reason is that this algorithm already has a property CorrectionAlgorithm which scientists usually refer to as just "corrections". This property has two possible values (apart from None): logarithmic and exponential. I think if they see CorrectionType they could think it refers to the algorithm corrections. For this reason I think a better name would contain the work "detectors" (DetectorCorrectionType or something like that), this way it is clear that this property refers to detectors. The same applies to the interface. Because we don't want to have different names for the same property in different algorithms, I would rename it in SpecularReflectionPositionCorrect too.

In addition, I see in the interface that the property is in the group Experiment Settings. I think it should be in Instrument Settings, because a specific instrument always moves the detectors in a specific way, as far as I know. For instance, my understanding is that in INTER detectors are moved vertically, whereas in POLREF and OFFSPEC they are rotated around the sample. Ideally, the interface would default this property to the appropriate value depending on the selected instrument, but that is for a separate issue, if anyone requests it.

[raquelalvarezbanos]

Can we add a brief sentence when we mention algorithm SpecularReflectionPositionCorrect saying that detectors will be corrected according to the new property? The reason I suggest this is because I know most of the times scientists only look at the documentation of ReflectometryReductionOneAuto.

[raquelalvarezbanos]

Does this mean that in this case and for this angle, there's no difference between moving detectors vertically and rotating them?

[gemmaguest]

I think the difference is just so tiny it doesn't show up, but I'll check...

[gemmaguest]

Here's the output to 10dp:

point-detector
Original: 25.6000000000,0.0000000000,0.0444960632          
Vertical: 25.6000000000,0.0000000000,0.0444753266          
Rotated: 25.6000003547,0.0000000000,0.0444753326 

So, there is a difference but it's tiny. I can add a note to the docs to make that clear - it's probably useful to explain that sometimes the difference will be negligible.

[raquelalvarezbanos]

👍 Thanks for checking this, I just wanted to make sure that there was an explanation for this.

[raquelalvarezbanos]

While testing this I noticed a bug in the algorithm, which is unrelated to the changes in this PR. I was trying to run this script:

inter = Load(Filename='INTER13460')
inter_vert = SpecularReflectionPositionCorrect(inter, TwoTheta = 2*0.49, DetectorComponentName='lineardetector')
inter_rot = SpecularReflectionPositionCorrect(inter, TwoTheta = 2*0.49, DetectorComponentName='lineardetector', CorrectionType='RotateAroundSample')

which is obviously wrong because INTER does not have a component lineardetector. I get a segmentation fault on Ubuntu. Could you have a look and see if you get the same, and if so, create an issue to solve this? As you may now have more interesting things to work on, feel free to assign it to me or Pranav.

[gemmaguest]

I see the same crash with the invalid detector name, so I've created #18905 for that issue.

[gemmaguest]

I think I've made all of the changes suggested if you could take another look @raquelalvarezbanos, thanks.

[raquelalvarezbanos]

Thanks @gemmaguest , the new property works as expected, so this is ready to be merged, but some conflicts with master need to be resolved.

[raquelalvarezbanos]

when all tests pass.