RotateSampleShape algorithm created #37908

warunawickramasingha · 2024-09-03T13:13:33Z

Description of work

Created a new algorithm named RotateSampleShape to define the initial orientation of a sample wrt the beam and instrument.

Summary of work

Using RotateSampleShape the sample shape can be rotated by the goniometer before being used in the calculation of various attenuation corrections. It would change the orientation of a sample on a workspace and work for CSG shapes (e.g. cylinders, flat plates etc.) and Mesh files.

Fixes #34565.

To test:

Test 1:

RotateSampleShape for a workspace containing a CSG sample shape:

Run below code

ws = CreateSampleWorkspace()
shape_xml = " \
<hollow-cylinder id='A'>\
  <centre-of-bottom-base r='0.0' t='0.0' p='0.0' />  \
  <axis x='0.0' y='1.0' z='0' />\
  <inner-radius val='0.007' />\
  <outer-radius val='0.01' />\
  <height val='0.05' />\
</hollow-cylinder>\
"
SetSample(InputWorkspace=ws,
          Geometry={'Shape': 'CSG', 'Value': shape_xml},
          Material={'NumberDensity': 0.02, 'AttenuationXSection': 0.0,
                    'CoherentXSection': 0.0, 'IncoherentXSection': 0.0, 'ScatteringXSection': 80.0})

Observe the sample shape and orientation by right clicking on the ws ->Show Sample Shape
Now run the RotateSampleShape algorithm and observe the new orientation of the sample.

RotateSampleShape(Workspace=ws, Axis0="45,1,0,0,1")

Repeat the test with different shape_xml as below

shape_xml = " \
<cuboid id='some-cuboid'>\
  <width val='2.0' />\
  <height val='4.0'  />\
  <depth  val='0.2' />\
  <centre x='10.0' y='10.0' z='10.0'  />\
</cuboid>\
<algebra val='some-cuboid' /> \
"

shape_xml = " \
<hexahedron id='Bertie'>\
  <left-back-bottom-point  x='0.0' y='0.0' z='0.0'  />\
  <left-front-bottom-point x='1.0' y='0.0' z='0.0'  />\
  <right-front-bottom-point x='1.0' y='1.0' z='0.0'  />\
  <right-back-bottom-point  x='0.0' y='1.0' z='0.0'  />\
  <left-back-top-point  x='0.0' y='0.0' z='2.0'  />\
  <left-front-top-point  x='0.5' y='0.0' z='2.0'  />\
  <right-front-top-point  x='0.5' y='0.5' z='2.0'  />\
  <right-back-top-point  x='0.0' y='0.5' z='2.0'  />\
</hexahedron>\
"

Test 2:

RotateSampleShape for a workspace containing a Mesh shape:

Run below code

ws = CreateSampleWorkspace()
LoadSampleShape(InputWorkspace=ws, OutputWorkspace=ws.name(),  Filename=r"correct\filepath\here\Anvil.stl")

Observe the sample shape and orientation by right clicking on the ws ->Show Sample Shape
Run the RotateSampleShape algorithm and observe the new orientation of the sample.

RotateSampleShape(Workspace=ws, Axis0="45,0,1,0,1")

Test 3:

RotateSampleShape supports upto 6 axes of rotations. Play around with different combination of axes and configurations to observe the resulting orientation of the sample. Observe the resulting sample orientation when there is already a goniometer set with SetGoniometer as below.

ws = CreateSampleWorkspace()
SetGoniometer(Workspace=ws, Axis0="45,1,1,0,1")
SetSample(ws,
          Geometry={'Shape': 'Cylinder', 'Height': 4.0,
                    'Radius': 1.0,
                    'Center': [0.,0.,0.]},
          Material={'ChemicalFormula': '(Li7)2-C-H4-N-Cl6',
                    'NumberDensity': 0.1})
RotateSampleShape(Workspace=ws, Axis0="45,0,0,1,1")

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.

RichardWaiteSTFC

Here's some comments - sorry haven't been able to test yet getting boost errors which i think means I need to do a clean rebuild...

Framework/Crystal/src/RotateSampleShape.cpp

RichardWaiteSTFC · 2024-09-06T15:17:14Z

Framework/Crystal/src/RotateSampleShape.cpp

+  return false;
+}
+
+void RotateSampleShape::prepareGoniometerAxes(Goniometer &gon, const API::ExperimentInfo_sptr &ei) {


This appears to be the same as SetGoniometer code - I wonder if it's possible to refactor and put in a place it can be called from both algorithms? Like a goniometer helperfile?

Although they appear to be the same code, there are some differences in terms of some additional validations, throw errors I have added in RotateSampleShape 's code and in the below line(angle was set as 0 in SetGoniometer alg)

gon.pushAxis(axisName, x, y, z, angle, ccw);

If you think those validations are better to be included in SetGoniometer algorithm as well, then I can move to a utility class.

OK, is there any reason why the validation has to be different in this case? If so then I'm OK to duplicate some code here - I definitely don't want to change the behaviour of SetGoniometer)

1- Validation was done a little bit different to give more detailed information to the user such as

if (!Strings::convert(tokens[1], x)) throw std::invalid_argument("Error converting x string '" + tokens[1] + "' to a number.");

vs

if (!Strings::convert(tokens[1], x)) throw std::invalid_argument("Error converting string '" + tokens[1] + "' to a number.");

2- For SetGoniometer the below is a valid axis but not valid for RotateSampleShape as it needs the rotation angle as an input and hence needs a different validation.
Axis0="Motor1,0,1,0,1"

3- The name of the TimeSeriesProperty(axisName) added as a log value in RotateSampleShape below needs to differ from SetGoniometer as well to keep them separate when both algorithms are used for the same sample(which can be an input to helper class).

if (ei->mutableRun().hasProperty(axisName)) { ei->mutableRun().removeLogData(axisName);

OK sounds good thanks!

docs/source/algorithms/RotateSampleShape-v1.rst

RichardWaiteSTFC · 2024-09-06T15:20:01Z

docs/source/algorithms/RotateSampleShape-v1.rst

+                        'Center': [0.,0.,0.]},
+            Material={'ChemicalFormula': '(Li7)2-C-H4-N-Cl6',
+                        'NumberDensity': 0.1})
+    RotateSampleShape(Workspace=ws, Axis0="45,1,1,0,1", Axis1="15,0,0,1,-1")


Would it be overkill to print out some shape xml in the test code so the user can see something has actually changed?

I think this has broken the doctests

Sorry! Is that because we don't have a .. testoutput:: block? Doesn't need to be a doc test, can just make it

**Example:** .. code-block:: python

RichardWaiteSTFC

Functional testing done - it works well!

Co-authored-by: RichardWaiteSTFC <[email protected]>

…mantid into 1643930_RotateSampleShape

RichardWaiteSTFC · 2024-09-10T11:08:35Z

Framework/Crystal/src/RotateSampleShape.cpp

+        if (ei->mutableRun().hasProperty(axisName)) {
+          ei->mutableRun().removeLogData(axisName);
+        }
+        ei->mutableRun().addLogData(tsp);


Looking at this again, I don't think it's necessary to add the gonio axis to the logs as in SetGoniometer - here everything we need is saved on the sample shape/xml right?

If it is not necessary to add this gonio axis log values (with a different name opposed to SetGoniometer) this bit can be removed, since it does not affect the functionality.

RichardWaiteSTFC · 2024-09-10T14:34:28Z

Changes look good, just an unused parameter causing clang failure. Once fixed I will approve!

warunawickramasingha · 2024-09-10T15:08:17Z

Changes look good, just an unused parameter causing clang failure. Once fixed I will approve!

Where can I see this Clang failure? locally it builds fine to me.

RichardWaiteSTFC · 2024-09-10T16:37:26Z

Changes look good, just an unused parameter causing clang failure. Once fixed I will approve!

Where can I see this Clang failure? locally it builds fine to me.

If you look at the OSX build it says Clang: One warning - if you click on that it shows you

sf1919 · 2024-09-11T08:10:08Z

@warunawickramasingha I won't re-run the linux tests as it looks like you need to make another commit and it will re-run then.

warunawickramasingha · 2024-09-11T12:00:20Z

Changes look good, just an unused parameter causing clang failure. Once fixed I will approve!

Where can I see this Clang failure? locally it builds fine to me.

If you look at the OSX build it says Clang: One warning - if you click on that it shows you

Thanks for the reviews @RichardWaiteSTFC! This has been fixed and now the CI is green

RichardWaiteSTFC

Looks great thanks!

That PR causes a dynamic library loading problem in the release.

Revert #37908

This reverts commit 6bac453.

warunawickramasingha added the ISIS Team: Diffraction Issue and pull requests managed by the Diffraction subteam at ISIS label Sep 3, 2024

warunawickramasingha added this to the Release 6.11 milestone Sep 3, 2024

warunawickramasingha force-pushed the 1643930_RotateSampleShape branch from 1ffe77f to d8a6acc Compare September 3, 2024 15:13

warunawickramasingha changed the title ~~1643930 RotateSampleShape v1~~ RotateSampleShape algorithm created Sep 3, 2024

SilkeSchomann and others added 2 commits September 3, 2024 16:16

Trigger Build

e6cb934

Rotate Sample shape done with unit tests and docs

f96da9d

warunawickramasingha force-pushed the 1643930_RotateSampleShape branch from d8a6acc to f96da9d Compare September 3, 2024 15:16

warunawickramasingha added 2 commits September 4, 2024 09:27

doc test fix

8485e97

release notes added

1ee23f8

RichardWaiteSTFC requested changes Sep 6, 2024

View reviewed changes

RichardWaiteSTFC self-assigned this Sep 9, 2024

RichardWaiteSTFC reviewed Sep 9, 2024

View reviewed changes

warunawickramasingha and others added 3 commits September 10, 2024 09:49

Update Framework/Crystal/src/RotateSampleShape.cpp

4d15d09

Co-authored-by: RichardWaiteSTFC <[email protected]>

print added to docs test code

74e6800

Merge branch '1643930_RotateSampleShape' of github.com:mantidproject/…

e9b885c

…mantid into 1643930_RotateSampleShape

RichardWaiteSTFC requested changes Sep 10, 2024

View reviewed changes

removed log values for axis

a03defe

warunawickramasingha requested a review from RichardWaiteSTFC September 10, 2024 14:26

Fixed clang warning

faacdcf

RichardWaiteSTFC approved these changes Sep 11, 2024

View reviewed changes

SilkeSchomann self-assigned this Sep 12, 2024

SilkeSchomann merged commit b286479 into main Sep 12, 2024
10 checks passed

SilkeSchomann deleted the 1643930_RotateSampleShape branch September 12, 2024 06:52

sf1919 mentioned this pull request Sep 12, 2024

Increase external data timeout #37962

Merged

jclarkeSTFC added a commit to jclarkeSTFC/mantid that referenced this pull request Sep 30, 2024

Revert mantidproject#37908

6bac453

That PR causes a dynamic library loading problem in the release.

jclarkeSTFC mentioned this pull request Oct 1, 2024

Revert #37908 #38110

Merged

warunawickramasingha mentioned this pull request Oct 1, 2024

Update to cmakeLists.txt to remove Mantid::DataHandling #38113

Closed

thomashampson added a commit that referenced this pull request Oct 1, 2024

Merge pull request #38110 from jclarkeSTFC/fix_crystal_dependency

a931470

Revert #37908

warunawickramasingha added a commit that referenced this pull request Oct 1, 2024

Revert "Revert #37908"

f618ac9

This reverts commit 6bac453.

warunawickramasingha mentioned this pull request Oct 1, 2024

Re add rotatesampleshape #38115

Open

warunawickramasingha added a commit that referenced this pull request Oct 3, 2024

Revert "Revert #37908"

6667b5c

This reverts commit 6bac453.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RotateSampleShape algorithm created #37908

RotateSampleShape algorithm created #37908

warunawickramasingha commented Sep 3, 2024 •

edited

Loading

RichardWaiteSTFC left a comment

RichardWaiteSTFC Sep 6, 2024

warunawickramasingha Sep 10, 2024

RichardWaiteSTFC Sep 10, 2024 •

edited

Loading

warunawickramasingha Sep 10, 2024 •

edited

Loading

RichardWaiteSTFC Sep 10, 2024

RichardWaiteSTFC Sep 6, 2024

thomashampson Sep 12, 2024

RichardWaiteSTFC Sep 12, 2024

RichardWaiteSTFC left a comment

RichardWaiteSTFC Sep 10, 2024

warunawickramasingha Sep 10, 2024

RichardWaiteSTFC commented Sep 10, 2024

warunawickramasingha commented Sep 10, 2024

RichardWaiteSTFC commented Sep 10, 2024

sf1919 commented Sep 11, 2024

warunawickramasingha commented Sep 11, 2024

RichardWaiteSTFC left a comment

RotateSampleShape algorithm created #37908

RotateSampleShape algorithm created #37908

Conversation

warunawickramasingha commented Sep 3, 2024 • edited Loading

Description of work

Summary of work

To test:

Test 1:

Test 2:

Test 3:

Reviewer

Code Review

Functional Tests

Gatekeeper

RichardWaiteSTFC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardWaiteSTFC Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

warunawickramasingha Sep 10, 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

Choose a reason for hiding this comment

RichardWaiteSTFC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardWaiteSTFC commented Sep 10, 2024

warunawickramasingha commented Sep 10, 2024

RichardWaiteSTFC commented Sep 10, 2024

sf1919 commented Sep 11, 2024

warunawickramasingha commented Sep 11, 2024

RichardWaiteSTFC left a comment

Choose a reason for hiding this comment

warunawickramasingha commented Sep 3, 2024 •

edited

Loading

RichardWaiteSTFC Sep 10, 2024 •

edited

Loading

warunawickramasingha Sep 10, 2024 •

edited

Loading