Add polarizer efficiency algorithm #37180

jclarkeSTFC · 2024-04-17T14:58:10Z

New algorithm to calculate the supermirror polarizer efficiency using a group workspace with 4 runs for the 4 spin combinations, plus the efficiency of the analyser cell, calculated previously. This is part of the polarized SANS epic.

Output can either be to a workspace, a file, or both.

Fixes #36139

Summary of work

Added algorithm
Added tests
Added doc page

Further detail of work

Calculates how the efficiency of a polarizer varies with wavelength. The ordering of the workspaces in InputWorkspace is taken from the SpinStates parameter, and the analyser efficiency, e_cell, is given by AnalyserEfficiency.

The polarization of the polarizer, P_SM, is given by

Since the efficiency, e_SM , is given by (1 + P_SM)/2, we have that

To test:

This will generate a simple example:

wsPara = CreateSampleWorkspace('Histogram', Function='User Defined', UserDefinedFunction='name=UserFunction,Formula=0.5*exp(-0.0733*12*x*(1-0.9))',XUnit='Wavelength', xMin='1',XMax='8', BinWidth='1')
wsPara1 = CloneWorkspace(wsPara)
wsAnti = CreateSampleWorkspace('Histogram', Function='User Defined', UserDefinedFunction='name=UserFunction,Formula=0.5*exp(-0.0733*12*x*(1+0.9))',XUnit='Wavelength', xMin='1',XMax='8', BinWidth='1')
wsAnti1 = CloneWorkspace(wsAnti)

grp = GroupWorkspaces([wsPara,wsAnti,wsPara1,wsAnti1])
eCell = CreateSampleWorkspace('Histogram', Function='User Defined', UserDefinedFunction='name=UserFunction,Formula=(1 + tanh(0.0733 * 12 * x * 0.2))/2',XUnit='Wavelength', xMin='1',XMax='16', BinWidth='1')

psm = PolarizerEfficiency(grp, eCell)
print("Polarizer efficiency at a wavelength of " + str(mtd['psm'].dataX(0)[3]) + " Å is " + str(mtd['psm'].dataY(0)[3]))

Reviewer

Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.

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?
Do the release notes conform to the release notes guide?

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.

Gatekeeper

If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.

github-actions · 2024-04-24T13:06:06Z

👋 Hi, @jclarkeSTFC,

Conflicts have been detected against the base branch. Please rebase your branch against the base branch.

cailafinn

Code Review:

Have a few code notes, but it's looking good.

Functional testing:

Working well from both the script and the algorithm dialog. Tried a few different bad inputs and it handled them well.
The 1st column of the output data from the script you provided seems to provide an efficiency value of -1.108.... I was under the impression efficiencies should always be between 0 and 1, so I just want to check if that's expected or if I've not got confused about something.

Docs:

Look great! Thanks for including the maths for both the polarisation and the efficiency.

Framework/Algorithms/src/PolarizationCorrections/PolarizerEfficiency.cpp

jclarkeSTFC · 2024-05-01T15:34:27Z

Code Review:

* Have a few code notes, but it's looking good.

Functional testing:

* Working well from both the script and the algorithm dialog. Tried a few different bad inputs and it handled them well.

* The 1st column of the output data from the script you provided seems to provide an efficiency value of `-1.108...`. I was under the impression efficiencies should always be between 0 and 1, so I just want to check if that's expected or if I've not got confused about something.

Docs:

* Look great! Thanks for including the maths for both the polarisation and the efficiency.

Thanks, I think I've changed all the things you spotted, and I've updated to the new equation.

cailafinn

Functionally working well, just a couple more things I noticed. Apologies for not catching them in the first review.

Framework/Algorithms/src/PolarizationCorrections/PolarizerEfficiency.cpp

docs/source/algorithms/PolarizerEfficiency-v1.rst

github-actions · 2024-05-08T12:33:48Z

👋 Hi, @jclarkeSTFC,

Conflicts have been detected against the base branch. Please rebase your branch against the base branch.

Calculate polarizer efficiency using a group workspace with 4 runs for the 4 spin combinations, plus the efficiency of the analyser cell, calculated previously.

Missing const in a few places, deleted unneeded methods and used the operator overloads instead.

Includes updating expected test results

You can now either have an output workspace, or save to a file, or both.

Co-authored-by: Caila Finn <47181718+cailafinn@users.noreply.github.com>

This validator does not work when the type is specified explicitly as being a WorkspaceGroup, so instead we'll have to to check for wavelength in validateInputs().

cailafinn

Docs built and looking good. Code has a few more notes. Functionally it's working well for the normal use case, but there's a crash to desktop (segfault) when using an invalid workspace type for the Cell efficiency workspace.

cailafinn · 2024-05-10T12:25:56Z

Framework/Algorithms/src/PolarizationCorrections/PolarizerEfficiency.cpp

+  if (inputWorkspace->size() != 4) {
+    errorList[PropertyNames::INPUT_WORKSPACE] =
+        "The input group workspace must have four periods corresponding to the four spin configurations.";


It was pointed out (thanks @rbauststfc), that the formula for the calculation only requires two periods $T_{00}$ and $T_{01}$ as the flipper is off/removed to get the polariser's efficiency. Should we be restricting the number of workspaces in the group if the user might have a run with only the two analyser polarities.

cailafinn · 2024-05-10T12:29:10Z

Framework/Algorithms/src/PolarizationCorrections/PolarizerEfficiency.cpp

+void PolarizerEfficiency::validateGroupInput() {
+  const auto &results = validateInputs();
+  if (results.size() > 0) {
+    const auto &result = results.cbegin();
+    throw std::runtime_error("Issue in " + result->first + " property: " + result->second);
+  }
+}
+
+bool PolarizerEfficiency::processGroups() {
+  validateGroupInput();
+  calculatePolarizerEfficiency();
+  return true;
+}


I noticed during the flipper efficiency alg work that if the input property is explicitly a workspace group (like has now been done in this one) then these methods aren't strictly necessary as it expects a group during a normal exec. This may be worth testing as it simplifies the call stack for the validation and may help with that error reporting issue we noted before.

cailafinn · 2024-05-10T12:40:23Z

Framework/Algorithms/src/PolarizationCorrections/PolarizerEfficiency.cpp

+      }
+    }
+  }
+


We want some checks for the analyser efficiency workspace in here:

In wavelength

Is the same size (number of histograms) as the input workspaces

Is a MatrixWorkspace

As it stands, you can provide something like a workspaceGroup and crash when you cast it to a MatrixWorkspace on L133. We also want tests adding for this.

cailafinn · 2024-05-10T12:43:36Z

Framework/Algorithms/src/PolarizationCorrections/PolarizerEfficiency.cpp

+  const auto &groupWorkspace =
+      AnalysisDataService::Instance().retrieveWS<WorkspaceGroup>(getProperty(PropertyNames::INPUT_WORKSPACE));


May not be in the ADS. Get directly from the property.

Suggested change

const auto &groupWorkspace =

AnalysisDataService::Instance().retrieveWS<WorkspaceGroup>(getProperty(PropertyNames::INPUT_WORKSPACE));

const WorkspaceGroup_sptr inputWorkspace = getProperty(PropertyNames::INPUT_WORKSPACE);

On this, and relevant to the other comment about simplifying the validation, have we done some testing to see if you really can pass in a workspace that's not in the ADS to a WorkspaceGroup property? I did what I consider to be some very incomplete testing, but it seemed to show that the property type was enforcing that the workspace group had to be in the ADS. It would be good to establish this conclusively, as it will be relevant to all the other polarised SANS algorithms.

Yes, I can confirm this. Below shows the debugging from C++ in the tests. The CreateSampleWorkspace outputs are present in the ADS due to it being a non-child alg, but the group is not as the GroupWorkspaces alg is a child algorithm. The group only exists as a pointer in the C++ code, which is then passed to the FlipperEfficiency alg, without entering the ADS, and the alg is still able to process normally.

OK, brilliant, thanks for checking 👍

cailafinn · 2024-05-10T12:45:03Z

Framework/Algorithms/src/PolarizationCorrections/PolarizerEfficiency.cpp

+  auto effCell = convertToHistIfNecessary(
+      AnalysisDataService::Instance().retrieveWS<MatrixWorkspace>(getProperty(PropertyNames::ANALYSER_EFFICIENCY)));


As above.

Suggested change

auto effCell = convertToHistIfNecessary(

AnalysisDataService::Instance().retrieveWS<MatrixWorkspace>(getProperty(PropertyNames::ANALYSER_EFFICIENCY)));

auto effCell = convertToHistIfNecessary(getProperty(PropertyNames::INPUT_WORKSPACE));

rbauststfc · 2024-05-10T14:30:46Z

Just making a note that because we'll now be targeting 6.11 for this change, we'll need to rebase and move the release note to the appropriate 6.11 folder.

jclarkeSTFC added SANS Issues and pull requests related to SANS ISIS Team: LSS Issue and pull requests managed by the LSS subteam at ISIS labels Apr 17, 2024

jclarkeSTFC added this to the Release 6.10 milestone Apr 17, 2024

github-actions bot added the Has Conflicts Used by the bot to label pull requests that have conflicts label Apr 24, 2024

jclarkeSTFC force-pushed the 36139_polariser_efficiency branch from 96070c7 to e86fc0b Compare April 25, 2024 07:58

jclarkeSTFC removed the Has Conflicts Used by the bot to label pull requests that have conflicts label Apr 25, 2024

jclarkeSTFC force-pushed the 36139_polariser_efficiency branch from e86fc0b to 0355e62 Compare April 25, 2024 13:59

jclarkeSTFC marked this pull request as ready for review April 26, 2024 08:24

cailafinn self-assigned this Apr 29, 2024

cailafinn requested changes Apr 29, 2024

View reviewed changes

jclarkeSTFC force-pushed the 36139_polariser_efficiency branch from 897c81b to 209a674 Compare May 1, 2024 15:29

jclarkeSTFC force-pushed the 36139_polariser_efficiency branch 4 times, most recently from 4eba049 to c74397b Compare May 2, 2024 15:44

cailafinn requested changes May 3, 2024

View reviewed changes

Framework/Algorithms/src/PolarizationCorrections/PolarizerEfficiency.cpp Outdated Show resolved Hide resolved

Framework/Algorithms/src/PolarizationCorrections/PolarizerEfficiency.cpp Outdated Show resolved Hide resolved

docs/source/algorithms/PolarizerEfficiency-v1.rst Show resolved Hide resolved

github-actions bot added the Has Conflicts Used by the bot to label pull requests that have conflicts label May 8, 2024

jclarkeSTFC and others added 10 commits May 9, 2024 10:49

Add algorithm, tests, and docs

b31b9eb

Calculate polarizer efficiency using a group workspace with 4 runs for the 4 spin combinations, plus the efficiency of the analyser cell, calculated previously.

Fix doc test

36fa2b7

Update to use new formula for efficiency

8d8bc85

Pre-emptive attack on Coverity

65db887

Tidy up code

909639d

Missing const in a few places, deleted unneeded methods and used the operator overloads instead.

Update formula for polarizer efficiency

11e1794

Includes updating expected test results

Fix cppcheck

5096c96

Update equations in docs

d400136

Add option to save to file

9db3f69

You can now either have an output workspace, or save to a file, or both.

Specify input is a workspace group

bab74fa

Co-authored-by: Caila Finn <47181718+cailafinn@users.noreply.github.com>

jclarkeSTFC and others added 4 commits May 9, 2024 10:50

Input may not be in the ADS

48c6cfd

Co-authored-by: Caila Finn <47181718+cailafinn@users.noreply.github.com>

Remove validator on input workspace

153d06d

This validator does not work when the type is specified explicitly as being a WorkspaceGroup, so instead we'll have to to check for wavelength in validateInputs().

Add note about rebinning to documentation

387fd4e

Minor refactoring

19babc8

rbauststfc removed the Has Conflicts Used by the bot to label pull requests that have conflicts label May 9, 2024

rbauststfc force-pushed the 36139_polariser_efficiency branch from cc92f48 to 19babc8 Compare May 9, 2024 12:28

rbauststfc modified the milestones: Release 6.10, Release 6.11 May 9, 2024

cailafinn requested changes May 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add polarizer efficiency algorithm #37180

Add polarizer efficiency algorithm #37180

jclarkeSTFC commented Apr 17, 2024 •

edited

github-actions bot commented Apr 24, 2024

cailafinn left a comment

jclarkeSTFC commented May 1, 2024

Code Review:

Functional testing:

Docs:

cailafinn left a comment

github-actions bot commented May 8, 2024

cailafinn left a comment

cailafinn May 10, 2024

cailafinn May 10, 2024

cailafinn May 10, 2024

cailafinn May 10, 2024

rbauststfc May 10, 2024

cailafinn May 10, 2024 •

edited

rbauststfc May 10, 2024

cailafinn May 10, 2024

rbauststfc commented May 10, 2024

		const auto &groupWorkspace =
		AnalysisDataService::Instance().retrieveWS<WorkspaceGroup>(getProperty(PropertyNames::INPUT_WORKSPACE));

	const auto &groupWorkspace =
	AnalysisDataService::Instance().retrieveWS<WorkspaceGroup>(getProperty(PropertyNames::INPUT_WORKSPACE));
	const WorkspaceGroup_sptr inputWorkspace = getProperty(PropertyNames::INPUT_WORKSPACE);

		auto effCell = convertToHistIfNecessary(
		AnalysisDataService::Instance().retrieveWS<MatrixWorkspace>(getProperty(PropertyNames::ANALYSER_EFFICIENCY)));

Add polarizer efficiency algorithm #37180

Are you sure you want to change the base?

Add polarizer efficiency algorithm #37180

Conversation

jclarkeSTFC commented Apr 17, 2024 • edited

Summary of work

Further detail of work

To test:

Reviewer

Code Review

Functional Tests

Gatekeeper

github-actions bot commented Apr 24, 2024

cailafinn left a comment

Choose a reason for hiding this comment

Code Review:

Functional testing:

Docs:

jclarkeSTFC commented May 1, 2024

Code Review:

Functional testing:

Docs:

cailafinn left a comment

Choose a reason for hiding this comment

github-actions bot commented May 8, 2024

cailafinn left a comment

Choose a reason for hiding this comment

cailafinn May 10, 2024

Choose a reason for hiding this comment

cailafinn May 10, 2024

Choose a reason for hiding this comment

cailafinn May 10, 2024

Choose a reason for hiding this comment

cailafinn May 10, 2024

Choose a reason for hiding this comment

rbauststfc May 10, 2024

Choose a reason for hiding this comment

cailafinn May 10, 2024 • edited

Choose a reason for hiding this comment

rbauststfc May 10, 2024

Choose a reason for hiding this comment

cailafinn May 10, 2024

Choose a reason for hiding this comment

rbauststfc commented May 10, 2024

jclarkeSTFC commented Apr 17, 2024 •

edited

cailafinn May 10, 2024 •

edited