Ticket #9840 (closed: fixed)
ReflectometryReductionOne should utilise PolarizationCorrection
Reported by: | Owen Arnold | Owned by: | Harry Jeffery |
---|---|---|---|
Priority: | major | Milestone: | Release 3.3 |
Component: | Reflectometry | Keywords: | |
Cc: | Blocked By: | #8948, #10131 | |
Blocking: | #10205, #10655 | Tester: | Dan Nixon |
Description
This is a low risk change, because it should really just involve wiring the properties in, and adding an additional one to enable the polarization correction.
We want to be able to perform polarization corrections (using the new polarization correction algorithm) to ReflectometryReductionOne and ReflectometryReductionOneAuto.
Attachments
Change History
comment:3 Changed 6 years ago by Owen Arnold
- Status changed from assigned to inprogress
refs #9840. Wired it up.
Now using polarizationcorrection, but untested.
Changeset: e1998de400de23ed02a138c3525c4538237a0ae7
comment:4 Changed 6 years ago by Owen Arnold
refs #9840. Issue encountered with multiperiod.
All seems to be running so far, but the PolarizationCorrection called from ReflectometryReductionOne is failing because it is not being passed the full input group workspace. This is most likely because ReflectometryReductionOne inherits from ReflectometryWorkflowBase, which inherits from DataProcessingAlgorithm, which inherits from Algorithm. I cannot inject the proper group workpsace processing algorithm (via MultiPeriodGroupAlgorithm) into this chain easily without making major structural changes.
The suggested way forwards would be to modify MultiPeriodGroupAlgorithm so that it's working were available as static methods. so that ReflectometryReductionOne could override ProcessGroups and CheckGroups and then point to the static methods in MultiPeriodGroupAlgorithm without technically inheriting from it.
Changeset: 0f09a01e68221a41d47afd0ef34d57bda1e9362a
comment:5 Changed 6 years ago by Owen Arnold
Attached script + changes induce problem described above.
comment:6 Changed 6 years ago by Owen Arnold
TODO: PolarizationCorrection should also inherit from MultiPeriodGroupAlgorithm, which it currently doesn't do!
comment:7 Changed 6 years ago by Owen Arnold
- Milestone changed from Release 3.2 to Release 3.3
The changes involved here are too dangerous this close to the release. It will have to be done as part of 3.3
comment:12 Changed 6 years ago by Owen Arnold
Comment 4 is no longer a problem since I introduced #10131. We can now use the shared code in this class to achieve our aims. We do NOT need to inherit from MultiPeriodAlgorithm to do this any longer. See the implementation of MultiPeriodAlgorithm to see how one would do this for the reflectometry workflow algorithms.
comment:13 Changed 6 years ago by Harry Jeffery
Refs #9840 Allow ReflRedOneAuto to handle WorkspaceGroups
Changeset: aadb793a367bc457ebf2bb573f35e4285c128b72
comment:14 Changed 6 years ago by Harry Jeffery
Refs #9840 Add polarization correction inputs
Changeset: 269bdfc6e2b20122c05dc6a0b6b9e302c341eca3
comment:15 Changed 6 years ago by Owen Arnold
Harry, i've sent you an email about a silicon sample to test this with.
comment:16 Changed 6 years ago by Owen Arnold
I've just had a quick look at https://github.com/mantidproject/mantid/compare/feature/9840_reflectometryreductionone_polarizationcorrection
Regarding checkGroups()
- You also need to check that the input workspace is multiperiod, as well as whether it is a group workspace. Use the isMultiperiod() method on the WorkspaceGroup for that.
- I don't know why you're fetching the workspace out of the ADS? directly
comment:17 Changed 6 years ago by Harry Jeffery
Refs #9840 Fix typo in previous commit
Changeset: da0c61c7897732f84bee9013b6fe8927d822a1ac
comment:18 Changed 6 years ago by Harry Jeffery
Merge branch 'master' into feature/9840_reflectometryreductionone_polarizationcorrection
Refs #9840
This merge is to incorporate the changes from #10524 into #9840.
Changeset: 8f25e486e211ed09fd8399a4c4453e3ddef6f3ab
comment:19 Changed 6 years ago by Harry Jeffery
Refs #9840 Remove polref duct tape from ReflMainViewPresenter
Changeset: 6669f2f425ad26824dea391842511819238284d5
comment:20 Changed 6 years ago by Harry Jeffery
Refs #9840 Implement polarization correction
Known bug: History is not recorded correctly.
Changeset: 3e210693bae1ccb1a5db32babd8095f5d65e5527
comment:21 Changed 6 years ago by Harry Jeffery
Refs #9840 Don't use the ADS in ReflectometryReductionOneAuto
Changeset: 1233ec6878162f9c53caa85f247f2ab8cd6895f3
comment:22 Changed 6 years ago by Harry Jeffery
Revert "Refs #9840 Don't use the ADS in ReflectometryReductionOneAuto"
This reverts commit 1233ec6878162f9c53caa85f247f2ab8cd6895f3.
Changeset: c7e784e68d0e877ba6c9fae413266ab6eaa1ac60
comment:23 Changed 6 years ago by Harry Jeffery
Refs #9840 Tidy up execution order
We can group the IvsQ workspaces at the same time as the IvsLam ones
Changeset: 2d5ed6a2ca69c68d03a7e52704d8a5ce661adb6e
comment:24 Changed 6 years ago by Harry Jeffery
Refs #9840 Fix error when re-stitching polref results
Changeset: 4daab451b52535ca1f26a6e4cfcb8830ccc73638
comment:25 Changed 6 years ago by Harry Jeffery
- Status changed from inprogress to verify
- Resolution set to fixed
- Blocking 10655 removed
Testing
The new functionality is a little tricky to test as we haven't any data to hand that clearly demonstrates the intended behaviour. However, the new functionality is only accessible by manually setting some options in the options column, so it will only be used in its current state by those who already know what they're doing, and what the correct output would look like. If the behaviour is incorrect, it should not cause any regressions in any other use cases.
When combined with #10646 it's easier to test, as we can look at the overall history of the output workspace and check that the processing workflow makes sense.
The behaviour covered by this ticket will be tested before the release of Mantid v3.3, as instructed by #10655.
comment:28 Changed 6 years ago by Harry Jeffery
- Status changed from verify to closed
Merge branch 'master' into feature/9840_reflectometryreductionone_polarizationcorrection
Refs #9840
This merge is to incorporate the changes from #10524 into #9840.
Full changeset: 8f25e486e211ed09fd8399a4c4453e3ddef6f3ab
comment:29 Changed 6 years ago by Dan Nixon
Merge remote-tracking branch 'origin/feature/9840_reflectometryreductionone_polarizationcorrection'
Full changeset: 19fb27610c6f6924d5f29f4e12ca09dc4eb0e95e
comment:32 Changed 5 years ago by Stuart Campbell
This ticket has been transferred to github issue 10682
This is the last 'nail in the coffin' of the quick scripts, which is why I'd like to get it done as part of 3.2