Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Output SpinState to SampleLog in PolarizationCorrectionFredrikze Algorithm #38447

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add Output SpinState to SampleLog in PolarizationCorrectionFredrikze Algorithm #38447

wants to merge 4 commits into from

Conversation

yusufjimoh
Copy link
Contributor

@yusufjimoh yusufjimoh commented Nov 26, 2024 •
edited
Loading

Description of work

  • Added functions to include spin state information in the PolarizationCorrectionFredrikZe algorithm.
  • Modified mapOutputWorkspaceToSpinState to append output spin states to the sample log when requested.
  • Updated unit tests to verify that the order of output workspaces aligns with the specified output spin states and that spin state information is correctly recorded in the sample log.
  • Updated documentation to reflect the new functionality and clarify its usage

Fixes #38265

To test:

Test for PA

  1. Run the following script. It will output a group of workspaces called "out". Check that the sample log of each child workspace does not contain an entry with the name spin_state_ORSO.
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")
  1. Without clearing any of your workspaces, run the following:
with_log = PolarizationCorrectionFredrikze(run, eff, "PA", InputSpinStates="pp, pa, ap, aa", OutputSpinStates="pp, pa, ap, aa", AddSpinStateToLog=True)

Check that the sample log of each child workspace in the "with_log" group does contain an entry with the name spin_state_ORSO. The log values should be as follows:

  • with_log_1 workspace has value pp
  • with_log_2 workspace has value pm
  • with_log_3 workspace has value mp
  • with_log_4 workspace has value mm
  1. Test the functionality on the GUI using the same input workspaces and spinstates

  • when AddSpinStateToLog checkbox is not enabled: Check that the sample log of each child workspace does not contain an entry with the name spin_state_ORSO.

  • when addSpinStateToLog checkbox is enabled: Check that the sample log of each child workspace in the "with_log" group does contain an entry with the name spin_state_ORSO. The log values should be as follows:

  • with_log_1 workspace has value pp

  • with_log_2 workspace has value pm

  • with_log_3 workspace has value mp

  • with_log_4 workspace has value mm

Test for PNR

  1. clear your workspaces and run the following script. It will output a group of workspaces called "out". Check that the sample log of each child workspace does not contain an entry with the name spin_state_ORSO.
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")
  1. Without clearing any of your workspaces, run the following:
with_log = out = PolarizationCorrectionFredrikze(run, eff, "PNR", InputSpinStates="p, a", OutputSpinStates="p, a", AddSpinStateToLog=True)

Check that the sample log of each child workspace in the "with_log" group does contain an entry with the name spin_state_ORSO. The log values should be as follows:

  • with_log_1 workspace has value po
  • with_log_2 workspace has value mo
  1. Test the functionality on the GUI using the same input workspaces and spinstates

  • when AddSpinStateToLog checkbox is not enabled: Check that the sample log of each child workspace does not contain an entry with the name spin_state_ORSO.

  • when addSpinStateToLog checkbox is enabled: Check that the sample log of each child workspace in the "with_log" group does contain an entry with the name spin_state_ORSO. The log values should be as follows:

  • with_log_1 workspace has value po

  • with_log_2 workspace has value mo

@yusufjimoh yusufjimoh linked an issue Nov 26, 2024 that may be closed by this pull request
Add output spin state to sample log in PolarizationCorrectionFredrikze #38265
Open
@yusufjimoh yusufjimoh added Reflectometry Issues and pull requests related to reflectometry ISIS Team: LSS Issue and pull requests managed by the LSS subteam at ISIS labels Nov 26, 2024
@yusufjimoh yusufjimoh added this to the Release 6.12 milestone Nov 26, 2024
@yusufjimoh yusufjimoh force-pushed the 38265-add-output-spin-state-to-sample-log-in-polarizationcorrectionfredrikze branch from 43e7234 to ac010a2 Compare November 26, 2024 13:47
@adriazalvarez adriazalvarez self-assigned this Nov 27, 2024
@yusufjimoh yusufjimoh force-pushed the 38265-add-output-spin-state-to-sample-log-in-polarizationcorrectionfredrikze branch 2 times, most recently from be89651 to 499ef76 Compare November 29, 2024 09:57
@yusufjimoh
Add Output Spinstate to samplelog in PolarizationCorrection Fredrikze…
7b0178c 
… Algorithm

- added function to add spinstate to polarizationcorrection fredrikze algorithm
- modified mapoutputworkspacetospinstate to add outspinstate to samplelog if requested
- updated unit tests to check that the order of output workspaces matches the order of specified output spinstates in the samplelog
- updated documentation
@yusufjimoh yusufjimoh force-pushed the 38265-add-output-spin-state-to-sample-log-in-polarizationcorrectionfredrikze branch from 499ef76 to 7b0178c Compare November 29, 2024 10:19
@yusufjimoh yusufjimoh marked this pull request as ready for review December 3, 2024 09:50
adriazalvarez
adriazalvarez requested changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@adriazalvarez adriazalvarez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this feature to the correction algorithm. I have tested it with the example scripts and can confirm that the spin_state_ORSO log is correctly displayed when the AddSpinStateToLog is checked to true.
Thanks also for adding an extensive test coverage, I have added a few suggestions to improve readability of the tests, and I'm not sure whether we should separate the log checking for complete different tests instead of appending them to existing ones, but for the time being, see if you can work on these suggestions.

Framework/Algorithms/src/PolarizationCorrectionFredrikze.cpp Show resolved Hide resolved
Framework/Algorithms/test/PolarizationCorrectionFredrikzeTest.h Show resolved Hide resolved
Framework/Algorithms/test/PolarizationCorrectionFredrikzeTest.h Outdated Show resolved Hide resolved
Framework/Algorithms/test/PolarizationCorrectionFredrikzeTest.h Outdated Show resolved Hide resolved
Framework/Algorithms/test/PolarizationCorrectionFredrikzeTest.h Outdated Show resolved Hide resolved
Framework/Algorithms/test/PolarizationCorrectionFredrikzeTest.h Show resolved Hide resolved
Framework/Algorithms/test/PolarizationCorrectionFredrikzeTest.h Outdated Show resolved Hide resolved
Framework/Algorithms/test/PolarizationCorrectionFredrikzeTest.h Outdated Show resolved Hide resolved
Framework/Algorithms/test/PolarizationCorrectionFredrikzeTest.h Outdated Show resolved Hide resolved
Framework/Algorithms/test/PolarizationCorrectionFredrikzeTest.h Outdated Show resolved Hide resolved
@yusufjimoh
Update Test based on suggested changes from code
81588a8 
- remove redundant unit tests
- limited the use of hardcoded strings especially for strings used in multiple test functions
@yusufjimoh
refactored test to reduce code repetition
f63b384
adriazalvarez
adriazalvarez requested changes Dec 10, 2024
View reviewed changes
Copy link
Contributor

@adriazalvarez adriazalvarez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few more small changes

Framework/Algorithms/test/PolarizationCorrectionFredrikzeTest.h
Comment on lines +30 to +32

static const int PA_GROUP_SIZE = 4;
static const int PNR_GROUP_SIZE = 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static const int PA_GROUP_SIZE = 4;
static const int PNR_GROUP_SIZE = 2;
constexpr int PA_GROUP_SIZE = 4;
constexpr int PNR_GROUP_SIZE = 2;

Framework/Algorithms/test/PolarizationCorrectionFredrikzeTest.h
for (size_t orderIdx = 0; orderIdx < spinStateOrders.size(); ++orderIdx) {
alg->setProperty("InputSpinStates", spinStateOrders[orderIdx]);
alg->setProperty("OutputSpinStates", spinStateOrders[orderIdx]);
alg->setProperty("AddSpinStateToLog", true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
alg->setProperty("AddSpinStateToLog", true);

Framework/Algorithms/test/PolarizationCorrectionFredrikzeTest.h
for (size_t orderIdx = 0; orderIdx < spinStateOrders.size(); ++orderIdx) {
alg->setProperty("InputSpinStates", spinStateOrders[orderIdx]);
alg->setProperty("OutputSpinStates", spinStateOrders[orderIdx]);
alg->setProperty("AddSpinStateToLog", true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
alg->setProperty("AddSpinStateToLog", true);

Framework/Algorithms/test/PolarizationCorrectionFredrikzeTest.h
@@ -305,20 +327,38 @@ class PolarizationCorrectionFredrikzeTest : public CxxTest::TestSuite {
alg->execute(), std::invalid_argument &);
}

// Test various combinations of spin state orders
void test_various_spin_state_orders_for_PA() {
void test_valid_spin_state_order_for_PA() {
auto groupWS = createGroupWorkspace(4, 4, 1, 1);
auto alg = initializeAlgorithm(groupWS, "PA", "1,0,0,0");

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
alg->setProperty("AddSpinStateToLog", true);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Yusuf, just to clarify following our conversation, Adrian is suggesting here that you would have the following:

  void test_valid_spin_state_order_for_PA() {
    auto groupWS = createGroupWorkspace(4, 4, 1, 1);
    auto alg = initializeAlgorithm(groupWS, "PA", "1,0,0,0");
    alg->setProperty("AddSpinStateToLog", true);

This way we're setting the AddSpinStateToLog property to true, which this test requires, but we're doing it outside the for loop that we have later in the test. This approach should be more efficient because this property will have the same value for every iteration of the loop, so we don't need to keep setting it each time.

As we discussed, you could potentially add something to initializeAlgorithm to optionally set this property when required. However I think most of the tests in this class do not need to set this property, so it seems fine to leave it to be set separately. If you prefer to move it into initializeAlgorithm, though, then that would also be fine I think.

Framework/Algorithms/test/PolarizationCorrectionFredrikzeTest.h

void test_valid_spin_state_order_for_PNR() {
auto groupWS = createGroupWorkspace(2, 4, 1, 1);
auto eff = makeEfficiencies(create1DWorkspace(4, 1, 1), "1,0,0,0", "1,0,0,0");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
auto eff = makeEfficiencies(create1DWorkspace(4, 1, 1), "1,0,0,0", "1,0,0,0");

Framework/Algorithms/test/PolarizationCorrectionFredrikzeTest.h
auto groupWS = createGroupWorkspace(2, 4, 1, 1);
auto eff = makeEfficiencies(create1DWorkspace(4, 1, 1), "1,0,0,0", "1,0,0,0");
auto alg = initializeAlgorithm(groupWS, "PNR", "1,0,0,0");

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
alg->setProperty("AddSpinStateToLog", true);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rbauststfc rbauststfc rbauststfc left review comments

@adriazalvarez adriazalvarez adriazalvarez requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@adriazalvarez adriazalvarez

Labels
ISIS Team: LSS Issue and pull requests managed by the LSS subteam at ISIS Reflectometry Issues and pull requests related to reflectometry
Projects
None yet
Milestone
Release 6.12
Development

Successfully merging this pull request may close these issues.

Add output spin state to sample log in PolarizationCorrectionFredrikze
3 participants
@yusufjimoh @rbauststfc @adriazalvarez