- Added spin state order handling to PolarizationCorrectionFredrikze Correction Algorithm #37672

yusufjimoh · 2024-07-17T16:51:48Z

Description of work

Implemented InputSpinStateOrder and OutputSpinStateOrder properties.
Updated execPA functions to dynamically handle different spin state orders.
Extracted input and output spin state mapping logic into separate functions for better code organization and reusability.
Ensured backward compatibility by setting default spin state orders.
Added validation to ensure input workspace count matches the spin state entered
Updated PolarizationEfficiencyCor algorithm to handle spin states for the PolarizationCorrectionFredrikze algorithm
Updated Documentations

Purpose of work

#36981

To test:

Execute the PolarizationCorrectionFredrikze Correction Algorithm
Test with Input SpinState and/or Output Spin State, algorithm should run successfully
Test without input SpinState and/or Output Spin State, confirm it still executes successfully
Execute the PolarizationEfficiencyCor algorithm, running the PolarizationCorrectionFredrikze algorithm as a child. The algorithm should run successfully

sf1919 · 2024-08-21T05:44:19Z

This appears to have cppcheck failures which are unrelated to recent isues with the Mantid pipelines.

…algorithm - Implemented InputSpinStateOrder and OutputSpinStateOrder properties. - Updated execPA functions to dynamically handle different spin state orders. - Extracted input and output spin state mapping logic into separate functions for better code organization and reusability. - Ensured backward compatibility by setting default spin state orders. - Added validation to ensure input workspace count matches the spin state entered

…of spin state orders.

…he polarization correction helper splitSpinStateString

…nEfficiencyCor - added input and output spin states for fredrikze algorithm in polarizationefficiencycor algorithm - updated unit tests to test spin states in both algorithms - updated polarizationefficiencycor documentation - added a release note for the update

cailafinn

This is half a review as I didn't have time to do the functional testing, but I had a few code comments to mention from that half:

cailafinn · 2024-09-06T11:27:45Z

Framework/Algorithms/src/PolarizationCorrectionFredrikze.cpp

+namespace SPIN_STATES {
+const std::string PP("pp");
+const std::string PA("pa");
+const std::string AP("ap");
+const std::string AA("aa");
+const std::string P("p");
+const std::string A("a");
+
+} // namespace SPIN_STATES


These can be included from the PolarizationCorrectionsHelpers header as SpinStateConfigurationsFredrikze, so don't need to be repeated here.

https://github.com/mantidproject/mantid/blob/main/Framework/Algorithms/inc/MantidAlgorithms/PolarizationCorrections/PolarizationCorrectionsHelpers.h#L34

cailafinn · 2024-09-06T11:39:33Z

Framework/Algorithms/src/PolarizationCorrectionFredrikze.cpp

@@ -38,6 +50,10 @@ const std::string cApLabel("Ap");

 const std::string efficienciesLabel("Efficiencies");

+const std::string inputSpinStateOrderLabel("InputSpinStateOrder");


I'm inclined to suggest we refactor the property constants here to match the way the Wildes version does it. I.e we have a namespace called Prop and then have constants for all of the properties (including the InputWorkspace etc. ones that aren't here yet). The constants should be in INPUT_WORKSPACE style to make it clear that they're constants. You'll also need to make sure the instances where they're accessed using strings are replaced with these constants.

cailafinn · 2024-09-06T11:58:33Z

Framework/Algorithms/src/PolarizationCorrectionFredrikze.cpp

+
+  declareProperty(
+      inputSpinStateOrderLabel, "", spinStateValidator,
+      "The order of spin states in the input workspace group. TThe possible values are 'pp,pa,ap,aa' or 'p,a'.");


Typo

Suggested change

"The order of spin states in the input workspace group. TThe possible values are 'pp,pa,ap,aa' or 'p,a'.");

"The order of spin states in the input workspace group. The possible values are 'pp,pa,ap,aa' or 'p,a'.");

cailafinn · 2024-09-06T12:01:18Z

Framework/Algorithms/src/PolarizationCorrectionFredrikze.cpp

+ * @param defaultOrder :: The vector of strings representing the default order.
+ * @return A map of spin state to MatrixWorkspace shared pointers.
+ */
+std::map<std::string, MatrixWorkspace_sptr> mapOrderToWorkspaces(const WorkspaceGroup_sptr &inWS,


Is there a reason these weren't included as private methods of the class in the header?

cailafinn · 2024-09-06T12:08:21Z

Framework/Algorithms/src/PolarizationCorrectionFredrikze.cpp

+                                         const std::vector<std::string> &order,
+                                         const std::vector<std::string> &defaultOrder) {
+  auto dataOut = std::make_shared<WorkspaceGroup>();
+  // If the order vector is empty, use the default order
+  const std::vector<std::string> effectiveOrder = order.empty() ? defaultOrder : order;


I know it would result a very small amount of code duplication, but I think passing both an order and a default order is less readable than only passing an order and leaving the decision of what to pass to the caller. It's a mild breach of the Single Responsibility Principle for it to decide the order to use and then map the workspaces. Feel free to leave it as-is, just my thoughts.

Thank you for this. I agree that it’s best to pass an order and leave the decision to the caller in order to avoid breaching the Single Responsibility Principle (SRP), code has been refactored to accommodate the suggested changes.

cailafinn · 2024-09-06T16:23:22Z

docs/source/algorithms/PolarizationCorrectionFredrikze-v1.rst

 Performs wavelength polarization correction on a TOF reflectometer spectrometer.

-Algorithm is based on the paper Fredrikze, H, et al. "Calibration of a polarized neutron reflectometer" Physica B 297 (2001).
+The algorithm is based on the paper Fredrikze, H, et al. "Calibration of a polarized neutron reflectometer" Physica B 297 (2001).


Could we add an explicit reference here? (The doc for DepolarizedAnalyserTransmission.rst has an example of how to do this.

cailafinn · 2024-09-06T16:28:15Z

docs/source/algorithms/PolarizationCorrectionFredrikze-v1.rst

+   Full polarization corrections: both parallel, parallel-anti-parallel, anti-parallel-parallel, both  anti-parallel. Four input workspaces are required. The spin states can be provided in any order and should match the order of the workspaces in the input group.
+
+- :literal:`'p, a'`
+   PNR polarization corrections:  parallel, anti-parallel. Two input workspaces are required. The spin states can be provided in any order and should match the order of the workspaces in the input group.


Can we make clear here that PA is Full polarization corrections and what PNR stands for?

cailafinn · 2024-09-06T16:30:34Z

docs/source/algorithms/PolarizationCorrectionFredrikze-v1.rst

-In the ideal case :math:`P_{p} = P_{a} = A_{p} = A_{a} = 1`
+Spin State Configurations
+#########################
+The algorithm directly uses spin states to define the measurement setup. The spin states are specified using two properties:


I think this reads a little better:

Suggested change

The algorithm directly uses spin states to define the measurement setup. The spin states are specified using two properties:

The algorithm expresses the order of the workspaces in the input and output groups in terms of spin states. These orders are specified using two properties:

cailafinn · 2024-09-06T16:32:19Z

docs/source/algorithms/PolarizationCorrectionFredrikze-v1.rst

+    the default order used by the algorithm if none is specified are :literal:`pp, pa, ap, aa` for full polarization analysis or :literal:`p, a` for PNR.
+
+**Output Spin State Order:**
+This property determines the order of the spin states in the output workspace group. Similar to the input configuration, users can specify output spin states in any order.

 Output from Full Polarization Analysis


Can we have an extra heading for the outputs from the PNR version?

cailafinn · 2024-09-06T16:32:54Z

docs/source/algorithms/PolarizationCorrectionFredrikze-v1.rst

+
+---
+


Is there a reason for this extra line being included?

cailafinn

Tested functionally with the following script. Changes are working well for the input and output spin state orders when the right number of characters are used, setting both properly.

# import mantid algorithms, numpy and matplotlib
from mantid.simpleapi import *
import matplotlib.pyplot as plt
import numpy as np

run = Load("POLREF00004699.nxs")
eff = Load("fredrikze_efficiencies_37125.nxs")
run = ConvertUnits(run, "Wavelength")
eff = ConvertUnits(eff, "Wavelength")
eff = RebinToWorkspace(eff, run.getItem(0), True)
out = PolarizationCorrectionFredrikze(run, eff, "PNR", OutputSpinStateOrder="p, a")

When messing with the script, found a couple of other issues:
Setting the OutputSpinStateOrder to "pp, aa" ran the algorithm and threw an IndexError: map::at: key not found, this should be checked in the validateInputs() method.
Setting the InputSpinStateOrder to "pp, aa" caused a segfault that crashed to desktop.

# import mantid algorithms, numpy and matplotlib
from mantid.simpleapi import *
import matplotlib.pyplot as plt
import numpy as np

run1 = Load("POLREF00004699.nxs")
run2 = Load("POLREF00004699.nxs")
run2 = run2 * 1.2
run = GroupWorkspaces([run1, run2])

eff = Load("fredrikze_efficiencies_37125.nxs")

run = ConvertUnits(run, "Wavelength")
eff = ConvertUnits(eff, "Wavelength")

eff = RebinToWorkspace(eff, run.getItem(0), True)

out = PolarizationCorrectionFredrikze(run, eff, "PA", OutputSpinStateOrder="pp, aa, pa, ap")

The inverse applies to the above, using p and a in this one.

yusufjimoh · 2024-09-11T13:10:51Z

Thank you for the comprehensive review. I have implemented the suggested changes and updated the unit tests to resolve the segfault crash

rbauststfc · 2024-09-12T14:09:14Z

@yusufjimoh, thanks for all the hard work on this one. It's a change that can definitely wait until the next release, that's no problem, so I'm going to switch the milestone to 6.12 at this point. It will mean that the release notes will need to be moved into a different folder at an appropriate stage (we can't do that just yet), so I'll discuss that with you at some point in the future (no need to worry about it right now). Hope this sounds OK.

cailafinn

Functionally tested using previous scripts and working well. Small issue with validation mentioned in code comments. Just a few more questions about the code, but this is looking good:

cailafinn · 2024-09-12T14:17:24Z

Framework/Algorithms/src/PolarizationCorrectionFredrikze.cpp

@@ -135,14 +196,14 @@ const std::string PolarizationCorrectionFredrikze::summary() const {
 * @param rhs : WS to multiply by (constant value)
 * @return Multiplied Workspace.
 */
-MatrixWorkspace_sptr PolarizationCorrectionFredrikze::multiply(MatrixWorkspace_sptr &lhsWS, const double &rhs) {
+MatrixWorkspace_sptr PolarizationCorrectionFredrikze::multiply(const MatrixWorkspace_sptr &lhsWS, double &rhs) {


Is there a reason the const was moved here rather than just making both const? If it's just a compilation thing, that's fine

cailafinn · 2024-09-12T14:19:21Z

Framework/Algorithms/src/PolarizationCorrectionFredrikze.cpp

-const std::string cAlphaLabel("Alpha");
+namespace Prop {
+static const std::string FLIPPERS{"Flippers"};
+static const std::string pNRLabel("PNR");


This and the other constants here should be capitalised.

You can also make these constexpr auto rather than explicitly declaring the string for slightly better runtime efficiency as constexpr means they'll be generated at compile time.

Suggested change

static const std::string pNRLabel("PNR");

constexpr auto PNR_LABEL("PNR");

cailafinn · 2024-09-12T14:27:32Z

Framework/Algorithms/src/PolarizationCorrectionFredrikze.cpp

+  if (analysisMode == Prop::pNRLabel) {
+    // for PR, spinStates must be "p,a", "a,p", or empty vector
+    return (spinStates.size() == 2 &&
+            ((spinStates[0] == "p" && spinStates[1] == "a") || (spinStates[0] == "a" && spinStates[1] == "p"))) ||


Could this be using the constants?

cailafinn · 2024-09-12T14:43:00Z

Framework/Algorithms/src/PolarizationCorrectionFredrikze.cpp

      "An output workspace.");
+
+  const auto spinStateValidator = std::make_shared<SpinStateValidator>(
+      std::unordered_set<int>{2, 4}, true, SpinStateConfigurationsFredrikze::PARA[0], 'a', true);


Is there a reason only one of the constants was used? I know it's expecting a char rather than a string (hence the [0]), but if we're using the constant like this or just 'p' can we be consistent between the para and anti?

cailafinn · 2024-09-12T14:54:28Z

Framework/Algorithms/src/PolarizationCorrectionFredrikze.cpp

+  const std::vector<std::string> inputStates = PolarizationCorrectionsHelpers::splitSpinStateString(inputStatesStr);
+  const std::vector<std::string> outputStates = PolarizationCorrectionsHelpers::splitSpinStateString(outputStatesStr);


I think you can use auto here.

cailafinn · 2024-09-12T14:58:28Z

Framework/Algorithms/src/PolarizationCorrectionFredrikze.cpp

  validateInputWorkspace(inWS);

+  if (!isValidSpinState(inputStates, analysisMode)) {
+    throw std::invalid_argument("Invalid input spin state: " + inputStatesStr + " for " + analysisMode);
+  }
+  if (!isValidSpinState(outputStates, analysisMode)) {
+    throw std::invalid_argument("Invalid output spin state: " + outputStatesStr + " for " + analysisMode);
+  }


For some reason Fredrikze doesn't seem to have it, but this kind of validation should be taking place in a valdiateInputs() method. See PolarizationCorrectionWildes.cpp for an example. This is also why these errors do not appear properly on the algorithm dialog.

These error messages should also probably identify what the valid inputs would look like.

cailafinn · 2024-09-12T15:08:13Z

Framework/Algorithms/src/PolarizationCorrectionFredrikze.cpp

+static const std::string OUTPUT_WORKSPACE{"OutputWorkspace"};
+static const std::string INPUT_SPIN_STATES("InputSpinStates");
+static const std::string OUTPUT_SPIN_STATES("OutputSpinStates");
+static const std::string crhoLabel("Rho");


I don't think this and some of the others in here (cApLabeletc.) are Properties of the algorithm, and so should probably be in a different namespace.

cailafinn · 2024-09-12T15:25:59Z

docs/source/algorithms/PolarizationCorrectionFredrikze-v1.rst

+Output from Polarized Neutron Reflectivity
+------------------------------------------
+
+The output of this algorithm is a :ref:`WorkspaceGroup <WorkspaceGroup>`, the resulting :ref:`WorkspaceGroup <WorkspaceGroup>` will have 2 entries, corresponding to the output specified spin state orders, if no specific output spin states are specified, it will be formatted as follows:


Suggested change

The output of this algorithm is a :ref:`WorkspaceGroup <WorkspaceGroup>`, the resulting :ref:`WorkspaceGroup <WorkspaceGroup>` will have 2 entries, corresponding to the output specified spin state orders, if no specific output spin states are specified, it will be formatted as follows:

The output of this algorithm is a :ref:`WorkspaceGroup <WorkspaceGroup>`. The resulting :ref:`WorkspaceGroup <WorkspaceGroup>` will have 2 entries, corresponding to the specified output spin state orders. If no specific output spin states are specified, it will be formatted as follows:

cailafinn · 2024-09-12T15:29:27Z

docs/source/algorithms/PolarizationCorrectionFredrikze-v1.rst

+.. testcode:: PolarizationCorrectionFredrikze (PNR)
+
+    # import mantid algorithms, numpy and matplotlib
+    from mantid.simpleapi import *
+    import matplotlib.pyplot as plt
+    import numpy as np
+
+    run = Load("POLREF00004699.nxs")
+    eff = Load("fredrikze_efficiencies_37125.nxs")
+    run = ConvertUnits(run, "Wavelength")
+    eff = ConvertUnits(eff, "Wavelength")
+    eff = RebinToWorkspace(eff, run.getItem(0), True)
+    out = PolarizationCorrectionFredrikze(run, eff, "PNR", InputSpinStates="pp, pa, ap, aa", OutputSpinStates="p, a")
+


There should probable be a .. testoutput:: here as in the one from the wildes docs. That way we can make sure this usage example consistently performs correctly.

cailafinn · 2024-09-12T15:30:10Z

docs/source/algorithms/PolarizationCorrectionFredrikze-v1.rst

+    run = ConvertUnits(run, "Wavelength")
+    eff = ConvertUnits(eff, "Wavelength")
+    eff = RebinToWorkspace(eff, run.getItem(0), True)
+    out = PolarizationCorrectionFredrikze(run, eff, "PNR", InputSpinStates="pp, pa, ap, aa", OutputSpinStates="p, a")


Does a PNR example typically take 4 workspaces an input or should it only take two?

rbauststfc

Looks like a lot of tidy-ups have gone into this PR, thanks very much for all of your hard work on this Yusuf!

I haven't completed functional testing yet, but I've checked the validation, which all seems very robust.

I've noticed a few places in the documentation and some of the validation messages where it might be good to clarify some of the wording, or where there are typos, so I've added relevant comments to look at those final bits, if that's OK.

I think that there are one or two places where we might like to look at our use of constants in this algorithm (mostly in the existing rather than new code), but given that quite a bit of refactoring and improvements have already been made in this PR, I will open a separate maintenance issue to look at these last bits.

Additionally, we've added unit tests that provide good coverage of the input and output spin state validation, which is great for now. As a follow-up maintenance task, I think it would be good to think about how we might test that the algorithm is applying the re-ordering correctly, as I think we're currently just assuming that if the algorithm doesn't throw an error then it's all worked OK. I will add a separate issue to look at this, as I don't want to hold up this PR too much longer.

rbauststfc · 2024-10-22T14:38:00Z

docs/source/algorithms/PolarizationEfficiencyCor-v1.rst

-for Fredrikze.
+The default values for the ``Flippers``, ``SpinStates``, and ``PolarizationAnalysis`` properties are empty strings, and
+correspond to the actual defaults of the child algorithms: ``00, 01, 10, 11`` / ``++, +-, -+, --`` for Wildes,
+and ``pp, pa, ap, aa`` / ``p, a`` for ``PA`` and ``PNR`` in Fredrikze.


I'm wondering if this sentence has got a bit confusing and might be better left simply as:

The default values for the ``Flippers``, ``SpinStates``, and ``PolarizationAnalysis`` properties are empty strings and correspond to the actual defaults of the child algorithms.

Users can then check the child algorithm documentation to clarify what default values are being applied.

rbauststfc · 2024-10-22T14:52:03Z

docs/source/algorithms/PolarizationCorrectionFredrikze-v1.rst

-The output of this algorithm, as we can see in the table of properties, is a :ref:`WorkspaceGroup <WorkspaceGroup>`. If the algorithm has been executed with "PA" as the Polarization mode
-then the resulting :ref:`WorkspaceGroup <WorkspaceGroup>` will have 4 entries and should be in the format:
+
+The output of this algorithm is a :ref:`WorkspaceGroup <WorkspaceGroup>`, the resulting :ref:`WorkspaceGroup <WorkspaceGroup>` will have 4 entries, corresponding to the output specified spin state orders, if no specific output spin states are specified, it will be formatted as follows:


It would be good for this wording to match the style of the PNR wording beneath it, so we would have the following here:

The output of this algorithm is a :ref:WorkspaceGroup . The resulting :ref:WorkspaceGroup will have 4 entries, corresponding to the specified output spin state orders. If no specific output spin states are specified, it will be formatted as follows:

rbauststfc · 2024-10-22T15:19:57Z

Framework/Algorithms/src/PolarizationCorrectionFredrikze.cpp

+
+  declareProperty(
+      Prop::INPUT_SPIN_STATES, "", spinStateValidator,
+      "The order of spin states in the input workspace group. The possible values are 'pp,pa,ap,aa' or 'p,a'.");


Could we add "in any order" to this property description to make sure it's clear, so we would have:

The order of spin states in the input workspace group. The possible values are 'pp,pa,ap,aa' or 'p,a', in any order.

rbauststfc · 2024-10-22T15:20:54Z

Framework/Algorithms/src/PolarizationCorrectionFredrikze.cpp

+
+  declareProperty(
+      Prop::OUTPUT_SPIN_STATES, "", spinStateValidator,
+      "The order of spin states in the input workspace group. The possible values are 'pp,pa,ap,aa' or 'p,a'.");


Could we add "in any order" to this property description to make sure it's clear. There's also a typo as we need to refer to the output workspace group here. With these changes, we would have:

The order of spin states in the output workspace group. The possible values are 'pp,pa,ap,aa' or 'p,a', in any order.

rbauststfc · 2024-10-22T15:36:34Z

Framework/Algorithms/src/PolarizationCorrectionFredrikze.cpp

+
+        if (!isValidSpinState(inputStates, analysisMode)) {
+          throw std::invalid_argument("Invalid input spin state: " + inputStatesStr + " for " + analysisMode +
+                                      " The possible values are 'pp,pa,ap,aa' or 'p,a' in any order");


Please could we change sentence:

" The possible values are 'pp,pa,ap,aa' or 'p,a' in any order"

To be:

" The possible values are 'pp,pa,ap,aa' for PA, or 'p,a' for PNR, in any order"

Both here and on line 140 where we validate the output spin states against the analysis mode?

This validation is really helpful, but when I was testing I noticed that if I used a PNR spin state with the PA analysis mode then the current error message doesn't make it clear what I need to change.

rbauststfc · 2024-10-22T16:06:30Z

Framework/Algorithms/src/PolarizationEfficiencyCor.cpp

@@ -210,7 +258,7 @@ void PolarizationEfficiencyCor::checkFredrikzeProperties() const {
  if (!isDefault(Prop::FLIPPERS)) {
    throw std::invalid_argument("Property Flippers cannot be used with the Fredrikze method.");
  }
-  if (!isDefault(Prop::SPIN_STATES)) {
+  if (!isDefault(Prop::OUTPUT_WILDES_SPIN_STATES)) {
    throw std::invalid_argument("Property SpinStates cannot be used with the Fredrikze method.");


I think this error message needs to say "Property SpinStatesOutWildes cannot be used with the Fredrikze method." now that the Wildes spin states property has been renamed.

rbauststfc · 2024-10-24T09:51:32Z

Thanks for the documentation updates Yusuf, those are all looking good.

Just to flag up, the docs build failure on this PR is genuine as the example code gives an error when run in workbench. I think the following should hopefully work OK. I would hold off updating this while I finish my functional testing, though, just to make sure this will be the last change. I will let you know when I've finished that.

For PNR:

run = Load("POLREF00004699.nxs")
ws1 = CreateWorkspace([0.01, 17.0, 34.0], [50000, 50000])
eff = JoinISISPolarizationEfficiencies(Pp=ws1,   Rho=ws1, Ap=ws1, Alpha=ws1)
run = ConvertUnits(run, "Wavelength")
eff = ConvertUnits(eff, "Wavelength")
eff = RebinToWorkspace(eff, run.getItem(0), True)

out = PolarizationCorrectionFredrikze(run, eff, "PNR", InputSpinStates="p, a", OutputSpinStates="p, a")

for i, orig in enumerate(run):
	corrected = out[i]
	y_idx = run[0].yIndexOfX(15.0)
	ratio = corrected.readY(29)[y_idx] / orig.readY(29)[y_idx]
	print(f"Ratio of corrected {corrected.name()} and original {orig.name()} intensity at 15A: {round(ratio, 4)}")

Which gives output:

   Ratio of corrected out_1 and original run_1 intensity at 15A: 1.0005
   Ratio of corrected out_2 and original run_2 intensity at 15A: 0.9508

For PA:

run1 = Load("POLREF00004699.nxs")
run2 = run1 * 1.2
run = GroupWorkspaces([run1, run2])
ws1 = CreateWorkspace([0.01, 17.0, 34.0], [50000, 50000])
eff = JoinISISPolarizationEfficiencies(Pp=ws1,   Rho=ws1, Ap=ws1, Alpha=ws1)
run = ConvertUnits(run, "Wavelength")
eff = ConvertUnits(eff, "Wavelength")
eff = RebinToWorkspace(eff, run.getItem(0), True)

out = PolarizationCorrectionFredrikze(run, eff, "PA", InputSpinStates="pp, pa, ap, aa", OutputSpinStates="pp, pa, ap, aa")

for i, orig in enumerate(run):
	corrected = out[i]
	y_idx = run[0].yIndexOfX(15.0)
	ratio = corrected.readY(29)[y_idx] / orig.readY(29)[y_idx]
	print(f"Ratio of corrected {corrected.name()} and original {orig.name()} intensity at 15A: {round(ratio, 4)}")

Which gives output:

   Ratio of corrected out_1 and original run1_1 intensity at 15A: 1.0025
   Ratio of corrected out_2 and original run1_2 intensity at 15A: 0.9527
   Ratio of corrected out_3 and original run2_1 intensity at 15A: 0.8355
   Ratio of corrected out_4 and original run2_2 intensity at 15A: 0.794

rbauststfc · 2024-10-24T11:32:03Z

Functional testing looks really good to me.👍 If we can just replace the example in the docs with the above then hopefully that should be the last thing left before this can go in.

- Renamed the input and output spin states - refactored tests to eliminate code duplication - added an helper function to fix crash which happens when user enter pp, aa as spinstates - improved code based on PR review suggestions - updated unit tests to handle crash which happens when user enter pp, aa as spinstates - updated documentation

yusufjimoh · 2024-10-24T11:49:27Z

Functional testing looks really good to me.👍 If we can just replace the example in the docs with the above then hopefully that should be the last thing left before this can go in.

Thanks for the documentation updates Yusuf, those are all looking good.

Just to flag up, the docs build failure on this PR is genuine as the example code gives an error when run in workbench. I think the following should hopefully work OK. I would hold off updating this while I finish my functional testing, though, just to make sure this will be the last change. I will let you know when I've finished that.

For PNR:

run = Load("POLREF00004699.nxs")
ws1 = CreateWorkspace([0.01, 17.0, 34.0], [50000, 50000])
eff = JoinISISPolarizationEfficiencies(Pp=ws1,   Rho=ws1, Ap=ws1, Alpha=ws1)
run = ConvertUnits(run, "Wavelength")
eff = ConvertUnits(eff, "Wavelength")
eff = RebinToWorkspace(eff, run.getItem(0), True)

out = PolarizationCorrectionFredrikze(run, eff, "PNR", InputSpinStates="p, a", OutputSpinStates="p, a")

for i, orig in enumerate(run):
	corrected = out[i]
	y_idx = run[0].yIndexOfX(15.0)
	ratio = corrected.readY(29)[y_idx] / orig.readY(29)[y_idx]
	print(f"Ratio of corrected {corrected.name()} and original {orig.name()} intensity at 15A: {round(ratio, 4)}")

Which gives output:

   Ratio of corrected out_1 and original run_1 intensity at 15A: 1.0005
   Ratio of corrected out_2 and original run_2 intensity at 15A: 0.9508

For PA:

run1 = Load("POLREF00004699.nxs")
run2 = run1 * 1.2
run = GroupWorkspaces([run1, run2])
ws1 = CreateWorkspace([0.01, 17.0, 34.0], [50000, 50000])
eff = JoinISISPolarizationEfficiencies(Pp=ws1,   Rho=ws1, Ap=ws1, Alpha=ws1)
run = ConvertUnits(run, "Wavelength")
eff = ConvertUnits(eff, "Wavelength")
eff = RebinToWorkspace(eff, run.getItem(0), True)

out = PolarizationCorrectionFredrikze(run, eff, "PA", InputSpinStates="pp, pa, ap, aa", OutputSpinStates="pp, pa, ap, aa")

for i, orig in enumerate(run):
	corrected = out[i]
	y_idx = run[0].yIndexOfX(15.0)
	ratio = corrected.readY(29)[y_idx] / orig.readY(29)[y_idx]
	print(f"Ratio of corrected {corrected.name()} and original {orig.name()} intensity at 15A: {round(ratio, 4)}")

Which gives output:

   Ratio of corrected out_1 and original run1_1 intensity at 15A: 1.0025
   Ratio of corrected out_2 and original run1_2 intensity at 15A: 0.9527
   Ratio of corrected out_3 and original run2_1 intensity at 15A: 0.8355
   Ratio of corrected out_4 and original run2_2 intensity at 15A: 0.794

I have updated the documentation with this, Thanks.

for more information, see https://pre-commit.ci

rbauststfc

Thanks very much for all your hard work on this issue Yusuf.

The code is working as expected and provides the additional functionality we needed under the original issue. There has also been a fair bit of maintenance pulled into this PR like some minor refactoring and adding usage examples to the docs, which are all looking good. There is some additional maintenance we could do and some unit test improvements that I think we could make, but it would be good to look at these under separate issues so that we can get this in and allow other related work to progress (see #38287 and #38288).

I'm dismissing Caila's stale review so that this can be gatekeepered - Caila's comments have either all been addressed or pulled into the linked maintenance issues.

…rrection Algorithm This is a squashed version of mantidproject#37672 - Added spin state order handling to PolarizationCorrectionFredrikze algorithm - Implemented InputSpinStateOrder and OutputSpinStateOrder properties. - Updated execPA functions to dynamically handle different spin state orders. - Extracted input and output spin state mapping logic into separate functions for better code organization and reusability. - Ensured backward compatibility by setting default spin state orders. - Added validation to ensure input workspace count matches the spin state entered - Added spin state order handling to PolarizationCorrectionFredrikze algorithm - Implemented InputSpinStateOrder and OutputSpinStateOrder properties. - Updated execPA functions to dynamically handle different spin state orders. - Extracted input and output spin state mapping logic into separate functions for better code organization and reusability. - Ensured backward compatibility by setting default spin state orders. - Added validation to ensure input workspace count matches the spin state entered - Included unit tests to verify correct functionality and validation of spin state orders. - splitted the spin state order string to a string of vectors using the polarization correction helper splitSpinStateString added p and a to spin states - modified unit tests to test spin state orders - updated the documentation to describe the spin states and its usage Add Input and Output SpinStates for FredrikzeAlgorithm in PolarizationEfficiencyCor - added input and output spin states for fredrikze algorithm in polarizationefficiencycor algorithm - updated unit tests to test spin states in both algorithms - updated polarizationefficiencycor documentation - added a release note for the update Refactor Fredrikze Algorithm SpinStates Implementation - Renamed the input and output spin states - refactored tests to eliminate code duplication - added an helper function to fix crash which happens when user enter pp, aa as spinstates - improved code based on PR review suggestions - updated unit tests to handle crash which happens when user enter pp, aa as spinstates - updated documentation --amend-no-edit [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci

yusufjimoh linked an issue Jul 17, 2024 that may be closed by this pull request

Allow bespoke input and output spin state in PolarizationCorrectionFredrikze #36981

Closed

2 tasks

yusufjimoh added 4 commits August 22, 2024 14:55

- Included unit tests to verify correct functionality and validation …

dcd3ede

…of spin state orders.

- splitted the spin state order string to a string of vectors using t…

1618052

…he polarization correction helper splitSpinStateString

yusufjimoh force-pushed the 36981-allow-bespoke-flipper-configurations-and-output-spin-state-in-polarizationcorrectionfredrikze branch 6 times, most recently from 45eb61c to 37d2200 Compare September 2, 2024 11:19

yusufjimoh added 3 commits September 3, 2024 11:15

added p and a to spin states

8dc2b71

- modified unit tests to test spin state orders

f0814c4

- updated the documentation to describe the spin states and its usage

732c037

yusufjimoh force-pushed the 36981-allow-bespoke-flipper-configurations-and-output-spin-state-in-polarizationcorrectionfredrikze branch from 37d2200 to 9e56a41 Compare September 3, 2024 10:17

yusufjimoh force-pushed the 36981-allow-bespoke-flipper-configurations-and-output-spin-state-in-polarizationcorrectionfredrikze branch from 9e56a41 to 2c7eef5 Compare September 4, 2024 09:17

yusufjimoh added ISIS Team: LSS Issue and pull requests managed by the LSS subteam at ISIS Reflectometry Issues and pull requests related to reflectometry labels Sep 4, 2024

yusufjimoh added this to the Release 6.11 milestone Sep 4, 2024

yusufjimoh marked this pull request as ready for review September 4, 2024 16:36

cailafinn self-assigned this Sep 6, 2024

cailafinn requested changes Sep 6, 2024

View reviewed changes

cailafinn requested changes Sep 9, 2024

View reviewed changes

rbauststfc modified the milestones: Release 6.11, Release 6.12 Sep 12, 2024

cailafinn previously requested changes Sep 12, 2024

View reviewed changes

yusufjimoh force-pushed the 36981-allow-bespoke-flipper-configurations-and-output-spin-state-in-polarizationcorrectionfredrikze branch 4 times, most recently from 69de188 to cb9eb1f Compare September 24, 2024 12:49

yusufjimoh force-pushed the 36981-allow-bespoke-flipper-configurations-and-output-spin-state-in-polarizationcorrectionfredrikze branch 4 times, most recently from 8e1542e to f1997c8 Compare October 3, 2024 18:08

yusufjimoh requested a review from cailafinn October 4, 2024 11:49

rbauststfc reviewed Oct 22, 2024

View reviewed changes

rbauststfc self-assigned this Oct 22, 2024

yusufjimoh force-pushed the 36981-allow-bespoke-flipper-configurations-and-output-spin-state-in-polarizationcorrectionfredrikze branch from f1997c8 to fe07a76 Compare October 24, 2024 07:14

This was referenced Oct 24, 2024

Maintenance tasks for PolarizationEfficiencyCor #38287

Closed

Maintenance tasks for PolarizationCorrectionFredrikze #38288

Closed

yusufjimoh force-pushed the 36981-allow-bespoke-flipper-configurations-and-output-spin-state-in-polarizationcorrectionfredrikze branch from fe07a76 to b2662b1 Compare October 24, 2024 11:40

yusufjimoh and others added 2 commits October 25, 2024 12:17

--amend-no-edit

a112825

[pre-commit.ci] auto fixes from pre-commit.com hooks

d2865fd

for more information, see https://pre-commit.ci

rbauststfc approved these changes Oct 25, 2024

View reviewed changes

rbauststfc unassigned cailafinn Oct 25, 2024

robertapplin self-assigned this Oct 28, 2024

robertapplin merged commit 4c2a82b into main Oct 28, 2024
10 checks passed

robertapplin deleted the 36981-allow-bespoke-flipper-configurations-and-output-spin-state-in-polarizationcorrectionfredrikze branch October 28, 2024 09:04

yusufjimoh mentioned this pull request Nov 1, 2024

Rename some HeliumAnalyserEfficiency algorithm parameters #38334

Merged

peterfpeterson mentioned this pull request Jan 16, 2025

Update cppcheck to 2.16.0 - ornlnext #38614

Merged

		@@ -38,6 +50,10 @@ const std::string cApLabel("Ap");

		const std::string efficienciesLabel("Efficiencies");

		const std::string inputSpinStateOrderLabel("InputSpinStateOrder");

	"The order of spin states in the input workspace group. TThe possible values are 'pp,pa,ap,aa' or 'p,a'.");
	"The order of spin states in the input workspace group. The possible values are 'pp,pa,ap,aa' or 'p,a'.");

	The algorithm directly uses spin states to define the measurement setup. The spin states are specified using two properties:
	The algorithm expresses the order of the workspaces in the input and output groups in terms of spin states. These orders are specified using two properties:

	static const std::string pNRLabel("PNR");
	constexpr auto PNR_LABEL("PNR");

		const std::vector<std::string> inputStates = PolarizationCorrectionsHelpers::splitSpinStateString(inputStatesStr);
		const std::vector<std::string> outputStates = PolarizationCorrectionsHelpers::splitSpinStateString(outputStatesStr);

	The output of this algorithm is a :ref:`WorkspaceGroup <WorkspaceGroup>`, the resulting :ref:`WorkspaceGroup <WorkspaceGroup>` will have 2 entries, corresponding to the output specified spin state orders, if no specific output spin states are specified, it will be formatted as follows:
	The output of this algorithm is a :ref:`WorkspaceGroup <WorkspaceGroup>`. The resulting :ref:`WorkspaceGroup <WorkspaceGroup>` will have 2 entries, corresponding to the specified output spin state orders. If no specific output spin states are specified, it will be formatted as follows:


		---

- Added spin state order handling to PolarizationCorrectionFredrikze Correction Algorithm #37672

- Added spin state order handling to PolarizationCorrectionFredrikze Correction Algorithm #37672

Conversation

yusufjimoh commented Jul 17, 2024 • edited Loading

Description of work

Purpose of work

To test:

sf1919 commented Aug 21, 2024

cailafinn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cailafinn Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cailafinn left a comment

Choose a reason for hiding this comment

yusufjimoh commented Sep 11, 2024

rbauststfc commented Sep 12, 2024

cailafinn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cailafinn Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbauststfc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbauststfc commented Oct 24, 2024

rbauststfc commented Oct 24, 2024

yusufjimoh commented Oct 24, 2024 • edited Loading

rbauststfc left a comment

Choose a reason for hiding this comment

yusufjimoh commented Jul 17, 2024 •

edited

Loading

cailafinn left a comment •

edited

Loading

cailafinn Sep 6, 2024 •

edited

Loading

cailafinn Sep 12, 2024 •

edited

Loading

yusufjimoh commented Oct 24, 2024 •

edited

Loading