Allow Determination of Workspace Order from Output of PolarizationCorrectionWildes #37643

cailafinn · 2024-07-08T15:51:07Z

Description of work

Summary of work

Adds a new SpinStates property to the algorithm that allows the user to select the order that the spin state (++, -+, etc.) workspaces appear in the output WorkspaceGroup.

Fixes #35067

Further detail of work

Adds the new property to the algorithm, ensuring that they appear in the right order when it has been set.
Adds validation to ensure that the correct number of states is provided for a given correction.
Amends the SpinStateValidator to handle SpinStates (++) and FlipperConfigurations (00).
Updates the documentation to account for the new functionality

To test:

Run the test from this documentation
Take note of the order of the workspaces in the output WorkspaceGroup.
Add a line under Flippers='00, 11' and add SpinStates=--,++,-+,+-`.
Run the script again.
Check the output workspace group is in the order you selected.

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.

yusufjimoh · 2024-08-13T13:34:18Z

Framework/Algorithms/test/PolarizationCorrectionWildesTest.h

+            return c * static_cast<double>(wsIndex + 1);
+          },
+          [&](size_t wsIndex, double c) {
+            if (pols[wsIndex] == "_++")


The code is well-structured, with a clear structure and effective use of lambda functions, which greatly enhance readability. The overall implementation is commendable and aligns with good coding practices.

One minor consideration might be to refactor the repeated if-else blocks into a dedicated function, possibly leveraging an std::unordered_map. This adjustment could reduce redundancy, and make it easier to manage the polarization strings if they need to be modified in the future.

Just a small thought that might be worth exploring:

size_t getPolIndex(const std::string &pol) { static const std::unordered_map<std::string, size_t> polMap = { {"_++", 0}, {"_+-", 1}, {"_-+", 2}, {"_--", 3} }; auto it = polMap.find(pol); if (it != polMap.end()) { return it->second; } else { throw std::invalid_argument("Unknown polarization string: " + pol); } }

yusufjimoh · 2024-08-14T13:14:46Z

Framework/Algorithms/src/PolarizationCorrectionWildes.cpp

+    names.emplace_back(baseName + "_" + spinState);
+    API::AnalysisDataService::Instance().addOrReplace(names.back(), ws);
+  } else {
+    auto const &i = PolarizationCorrectionsHelpers::indexOfWorkspaceForSpinState(spinStateOrder, spinState);


I encountered a build issue on line 559 that seems to stem from a type mismatch in the call to indexOfWorkspaceForSpinState. The function is expecting a const std::vector<std::string>& as its first argument, but it appears that a std::string is being passed instead

yusufjimoh · 2024-08-14T13:21:39Z

I've noticed both const auto and auto const being used in the code. Just wanted to understand if there's a difference between the two or if there's a particular reason for using them in those specific instances.

cailafinn · 2024-08-15T09:54:49Z

I've noticed both const auto and auto const being used in the code. Just wanted to understand if there's a difference between the two or if there's a particular reason for using them in those specific instances.

No, not functionally. This is the east const vs const west debate that has been going around in C++ circles for years. It's mostly stylistic in nature. I usually use east const by default as it's more consistent for what the const is referring to, but it's usually just best to keep to whatever convention is being used in the file you're editing.

I'll have a second look over the changes and make sure they're being consistent with the file.

yusufjimoh · 2024-08-15T17:24:06Z

That's interesting to know, Thanks for the explanation.

RE mantidproject#35067

There are a few different ways to represent the spin states of your flipper configurations (usually in inputs) and spin states (usually in outputs). Most notably between Wildes' method (+/-) and Fredrikze' method (p/a). The change allows the spin state validator to be initialised with any character to represent the on/off state of flippers or the parallel/antiparallel state of outputs. RE mantidproject#35067

RE mantidproject#35067

The way that the index finder helper works was changed in another PR. This accounts for that by handling the optional. The optional currently throws an error if it does not return an index, this is not testable as the validation currently prevents providing the spin states in such a way that this would be reached. But is implemented to result in a helpful error rather than a segfault should code changes result in problematic spin state strings getting through. RE mantidproject#35067

RE mantidproject#35067 Co-authored-by: Yusuf Jimoh <[email protected]>

yusufjimoh · 2024-08-19T13:15:51Z

The build issues have been resolved. I tested the code with and without the new spin states configuration, and it functions well in both scenarios for the PolarizationCorrectionWildes and PolarizationEfficiencyCor algorithms, respectively. The output workspace group was returned in the correct order. The tests added for the new spin states functionality are thorough.

Code is well-written
Functions correctly
Tests for the spin states are comprehensive
Documentation is updated

Everything appears to be in order.

yusufjimoh

I happened to notice that lines 559 and 132 might still be using auto const. not certain if that's intentional

Wildes file uses const west... *grumble* RE mantidproject#35067

RE mantidproject#35067

cailafinn · 2024-08-19T16:46:54Z

I happened to notice that lines 559 and 132 might still be using auto const. not certain if that's intentional

Good catch on 559. 132 matches the rest of the file.

yusufjimoh · 2024-08-19T17:13:51Z

Thanks for clarifying

sf1919 · 2024-08-20T12:13:09Z

Tests have now passed so reviewing can continue on this one.

yusufjimoh · 2024-08-20T16:03:26Z

All checks have been completed, and all tests are passing. Pull request approved

rbauststfc

Changes look really good and thanks very much for putting in the effort to make the spin state validator more generic, that will be extremely helpful for future work!

I noticed one or two minor things that I thought might be worth mentioning, although not all necessarily need changing if they were deliberate choices so feel free to go with your preference on those.

I can't seem to see any release notes on this PR, though, so it would be good to add some before merging this in.

Framework/Algorithms/inc/MantidAlgorithms/PolarizationCorrections/SpinStateValidator.h

Framework/Algorithms/src/PolarizationCorrectionWildes.cpp

rbauststfc

Sorry, I realise as this is at gatekeeper stage I'd better set to request changes to prevent merging before we have a release note.

RE mantidproject#35607

RE mantidproject#35067

github-actions · 2024-08-21T16:40:38Z

👋 Hi, @cailafinn,

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

The flipper configuration constants have been moved from the SpinValidator to the PolarizationCorrectionsHelpers file, so new algorithm PolarizationEfficienciesWildes needs to be updated to use these.

Changes have been made since the original review

rbauststfc · 2024-08-22T10:46:37Z

@cailafinn, the changes on this are looking really good, I've tested the changes to the validation and that's all working well. Thanks for adding a release note too. I'm afraid this PR of mine that was merged in yesterday caused some conflicting changes and I thought it seemed a bit unfair for you to have to sort those out when you got back, so I've resolved them, I hope that's OK.

@yusufjimoh, would you be happy to do a quick re-review (i.e. look through the code to confirm you're happy with the changes since your last review, but no need to re-do manual testing at this stage)? If you're happy then you can approve the PR as normal and I will gatekeeper it.

cailafinn added Reflectometry Issues and pull requests related to reflectometry SANS Issues and pull requests related to SANS Framework Issues and pull requests related to components in the Framework ISIS Team: LSS Issue and pull requests managed by the LSS subteam at ISIS labels Jul 8, 2024

cailafinn added this to the Release 6.11 milestone Jul 8, 2024

cailafinn marked this pull request as ready for review July 29, 2024 14:39

yusufjimoh self-assigned this Aug 9, 2024

yusufjimoh reviewed Aug 13, 2024

View reviewed changes

yusufjimoh reviewed Aug 14, 2024

View reviewed changes

cailafinn and others added 14 commits August 16, 2024 08:46

Distinguish between spin states and flipper configuration

be8f164

RE mantidproject#35067

Allow user to select ordering of output workspaces

321c603

RE mantidproject#35067

Fix output order failing for 3 inputs

3d9a3d5

RE mantidproject#35067

Correctly validate SpinStates parameter

3d6d0b5

RE mantidproject#35067

Update error messages in validator

53c9088

RE mantidproject#35067

Fix missing empty spinstates check

bacaf84

RE mantidproject#35067

Update documentation

cf81b71

RE mantidproject#35067

Remove unused states

6d4226b

RE mantidproject#35067

Add SpinStates property to wrapper alg

c68a9e8

RE mantidproject#35067

Mention new property in wrapper alg

09272fb

RE mantidproject#35067

Fix non-allowed <> tags in doxygen string

123331a

RE mantidproject#35067

Refactor and simplify pol index correction

8ce6029

RE mantidproject#35067 Co-authored-by: Yusuf Jimoh <[email protected]>

cailafinn force-pushed the 35067_wildes_flipper_config branch from 338435d to 7de1a34 Compare August 16, 2024 11:23

cailafinn requested a review from yusufjimoh August 19, 2024 08:25

yusufjimoh requested changes Aug 19, 2024

View reviewed changes

cailafinn added 2 commits August 19, 2024 17:28

Make const position consistent

542aba2

Wildes file uses const west... *grumble* RE mantidproject#35067

Make SeeAlso location consistent

7557939

RE mantidproject#35067

cailafinn force-pushed the 35067_wildes_flipper_config branch from 7de1a34 to 7557939 Compare August 19, 2024 16:46

cailafinn requested a review from yusufjimoh August 20, 2024 12:18

yusufjimoh previously approved these changes Aug 20, 2024

View reviewed changes

rbauststfc self-assigned this Aug 20, 2024

rbauststfc reviewed Aug 21, 2024

View reviewed changes

rbauststfc previously requested changes Aug 21, 2024

View reviewed changes

cailafinn added 3 commits August 21, 2024 11:09

Remove unneeded constsants from validator

29946af

RE mantidproject#35607

Correct allowed output spins

29d9fda

RE mantidproject#35607

Limit to two-character spin state outputs

06a6ca1

RE mantidproject#35067

cailafinn dismissed yusufjimoh’s stale review via 06a6ca1 August 21, 2024 11:08

Add a release note

0e375f9

RE mantidproject#35067

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

Merge branch 'main' into 35067_wildes_flipper_config

f988e58

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

rbauststfc added 2 commits August 22, 2024 08:54

Fix formatting of seeAlso method

8672d00

Use new constants in PolarizationEfficienciesWildes

0620675

The flipper configuration constants have been moved from the SpinValidator to the PolarizationCorrectionsHelpers file, so new algorithm PolarizationEfficienciesWildes needs to be updated to use these.

yusuphjoluwasen approved these changes Aug 22, 2024 •

edited by yusufjimoh

Loading

View reviewed changes

yusufjimoh approved these changes Aug 22, 2024

View reviewed changes

rbauststfc merged commit e7c101c into mantidproject:main Aug 22, 2024
10 checks passed

cailafinn deleted the 35067_wildes_flipper_config branch August 28, 2024 08:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Determination of Workspace Order from Output of PolarizationCorrectionWildes #37643

Allow Determination of Workspace Order from Output of PolarizationCorrectionWildes #37643

cailafinn commented Jul 8, 2024

yusufjimoh Aug 13, 2024

yusufjimoh Aug 14, 2024

yusufjimoh commented Aug 14, 2024

cailafinn commented Aug 15, 2024 •

edited

Loading

yusufjimoh commented Aug 15, 2024 •

edited

Loading

yusufjimoh commented Aug 19, 2024 •

edited

Loading

yusufjimoh left a comment •

edited

Loading

cailafinn commented Aug 19, 2024

yusufjimoh commented Aug 19, 2024

sf1919 commented Aug 20, 2024

yusufjimoh commented Aug 20, 2024

rbauststfc left a comment

rbauststfc left a comment

github-actions bot commented Aug 21, 2024

rbauststfc commented Aug 22, 2024

Allow Determination of Workspace Order from Output of PolarizationCorrectionWildes #37643

Allow Determination of Workspace Order from Output of PolarizationCorrectionWildes #37643

Conversation

cailafinn commented Jul 8, 2024

Description of work

Summary of work

Further detail of work

To test:

Reviewer

Code Review

Functional Tests

Gatekeeper

yusufjimoh Aug 13, 2024

Choose a reason for hiding this comment

yusufjimoh Aug 14, 2024

Choose a reason for hiding this comment

yusufjimoh commented Aug 14, 2024

cailafinn commented Aug 15, 2024 • edited Loading

yusufjimoh commented Aug 15, 2024 • edited Loading

yusufjimoh commented Aug 19, 2024 • edited Loading

yusufjimoh left a comment • edited Loading

Choose a reason for hiding this comment

cailafinn commented Aug 19, 2024

yusufjimoh commented Aug 19, 2024

sf1919 commented Aug 20, 2024

yusufjimoh commented Aug 20, 2024

rbauststfc left a comment

Choose a reason for hiding this comment

rbauststfc left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 21, 2024

rbauststfc commented Aug 22, 2024

cailafinn commented Aug 15, 2024 •

edited

Loading

yusufjimoh commented Aug 15, 2024 •

edited

Loading

yusufjimoh commented Aug 19, 2024 •

edited

Loading

yusufjimoh left a comment •

edited

Loading