Enable user defined sample and container geometry for abs correction #38887

Kvieta1990 · 2025-02-19T04:54:56Z

Description of work

Summary of work

With this PR, we enable user defined sample and container geometry for absorption correction. Top level changes are in place for SNSPowderReduction and accordingly necessary parameters are introduced for those bottom level algorithms concerning the setup of the absorption correction inputs. The defined sample and container geometry is finally passed to SetSample. This gives users the flexibility to define their own geometry as needed. For example, on POWGEN, some times irregular containers that are not defined in the sample environment XML file will be used for the measurement, e.g., the annular container where the sample is loaded into the hollow cylinder wall.

Also, we implement the definition of beam gauge volume for absorption correction. Without this implementation, the assumption would be that the beam size is the same as the sample size. In practice, this is often not true. In that case, the absorption correction is imperfect in principle. Detailed discussion about this is posted here.

To test:

This is the one for testing the gauge volume definition

from mantid.simpleapi import *

gauge_vol = '''<cuboid id="shape">
     <left-front-bottom-point x="0.01" y="-0.1" z="0.0"  />
     <left-front-top-point  x="0.01" y="-0.1" z="0.02"  />
     <left-back-bottom-point  x="-0.01" y="-0.1" z="0.0"  />
     <right-front-bottom-point  x="0.01" y="0.1" z="0.0"  />
   </cuboid>'''

SNSPowderReduction(
    Filename='/SNS/PG3/IPTS-34523/nexus/PG3_58812.nxs.h5',
    CalibrationFile='/SNS/PG3/shared/CALIBRATION/2025-1_11A_CAL/PG3_OC_d58772_2025-02-12.h5',
    CharacterizationRunsFile='/SNS/PG3/shared/CALIBRATION/2025-1_11A_CAL/PG3_char_2025_2-HighRes_OC.txt,/SNS/PG3/shared/CALIBRATION/2025-1_11A_CAL/PG3_char_2025_2_OC_limit.txt',
    RemovePromptPulseWidth=50,
    Binning='-0.0008',
    BackgroundSmoothParams='5, 2',
    FilterBadPulses=90,
    ScaleData=100,
    TypeOfCorrection='SampleAndContainer',
    SampleFormula='Ba-Nb-Cr-Ru',
    MeasuredMassDensity='6.0',
    SampleGeometry={'Height': 5},
    SampleNumberDensity='0.088',
    ContainerShape='PAC06',
    GaugeVolume=gauge_vol,
    SaveAs='gsas topas and fullprof',
    OutputDirectory='/SNS/NOM/shared/tmp',
    CacheDir='/SNS/NOM/shared/tmp'
)

Here is the one for testing the geometry definition,

from mantid.simpleapi import *

sam_geo = {
    'Shape': 'HollowCylinder', 'Height': 4.0,
    'InnerRadius': 2.0,
    'OuterRadius': 3.0,
    'Center': [0.,0.,0.]
}

c_geo = {
    'Shape': 'HollowCylinderHolder',
    'Height': 4.0,
    'InnerRadius': 1.5,
    'InnerOuterRadius': 2.0,
    'OuterInnerRadius': 3.0,
    'OuterRadius': 4.0,
    'Center': [0.,0.,0.]
}

c_mat = {
    'ChemicalFormula': 'V',
    'NumberDensity': 0.072324
}

gauge_vol = '''<cuboid id="shape">
     <left-front-bottom-point x="0.01" y="-0.1" z="0.0"  />
     <left-front-top-point  x="0.01" y="-0.1" z="0.02"  />
     <left-back-bottom-point  x="-0.01" y="-0.1" z="0.0"  />
     <right-front-bottom-point  x="0.01" y="0.1" z="0.0"  />
   </cuboid>'''

SNSPowderReduction(
    Filename='/SNS/PG3/IPTS-34523/nexus/PG3_58812.nxs.h5',
    CalibrationFile='/SNS/PG3/shared/CALIBRATION/2025-1_11A_CAL/PG3_OC_d58772_2025-02-12.h5',
    CharacterizationRunsFile='/SNS/PG3/shared/CALIBRATION/2025-1_11A_CAL/PG3_char_2025_2-HighRes_OC.txt,/SNS/PG3/shared/CALIBRATION/2025-1_11A_CAL/PG3_char_2025_2_OC_limit.txt',
    RemovePromptPulseWidth=50,
    Binning='-0.0008',
    BackgroundSmoothParams='5, 2',
    FilterBadPulses=90,
    ScaleData=100,
    TypeOfCorrection='SampleAndContainer',
    SampleFormula='Ba-Nb-Cr-Ru',
    MeasuredMassDensity='6.0',
    SampleGeometry=sam_geo,
    SampleNumberDensity='0.088',
    ContainerGeometry=c_geo,
    ContainerMaterial=c_mat,
    GaugeVolume=gauge_vol,
    SaveAs='gsas topas and fullprof',
    OutputDirectory='/SNS/NOM/shared/tmp',
    CacheDir='/SNS/NOM/shared/tmp'
)

Here is another one for testing the geometry definition, with only beam height defined (and therefore the gauge volume will be defined internally in Mantid by assuming the beam location),

from mantid.simpleapi import *
import matplotlib.pyplot as plt
import numpy as np

SNSPowderReduction(
    Filename='/SNS/PG3/IPTS-34523/nexus/PG3_58812.nxs.h5',
    CalibrationFile='/SNS/PG3/shared/CALIBRATION/2025-1_11A_CAL/PG3_OC_d58772_2025-02-12.h5',
    CharacterizationRunsFile='/SNS/PG3/shared/CALIBRATION/2025-1_11A_CAL/PG3_char_2025_2-HighRes_OC.txt,/SNS/PG3/shared/CALIBRATION/2025-1_11A_CAL/PG3_char_2025_2_OC_limit.txt',
    RemovePromptPulseWidth=50,
    Binning='-0.0008',
    BackgroundSmoothParams='5, 2',
    FilterBadPulses=90,
    ScaleData=100,
    TypeOfCorrection='SampleAndContainer',
    SampleFormula='Ba-Nb-Cr-Ru',
    MeasuredMassDensity='6.0',
    SampleGeometry={
        'Height': 5,
        'Radius': 0.295
    },
    BeamHeight=2,
    SampleNumberDensity='0.088',
    ContainerShape='PAC06',
    ContainerGaugeVolume='0.295 0.315',
    SaveAs='gsas topas and fullprof',
    OutputDirectory='/SNS/NOM/shared/tmp',
    CacheDir='/SNS/NOM/shared/tmp'
)

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.

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

…ntid into abs_can_geo_def

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

Framework/PythonInterface/mantid/utils/absorptioncorrutils.py

Kvieta1990 · 2025-02-21T16:05:32Z

@peterfpeterson I put in the implementation to define the gauge volume for sample and container separately to account for the beam size. I think you are right -- the way that gauge volume definition works is that the defined gauge volume will just replace the part of the sample shined by the beam. I did the test with a gauge volume larger than the sample size and in that case, as we change the gauge volume, the absorption spectrum is changing accordingly. This indicates that the gauge volume is not working as what I thought it would work, i.e., calculate the intersection region between the sample and the defined gauge volume.

As I put above, I think this is fine. We then just need to define the gauge volume to follow exactly the shape of the sample but with different height and position so only the defined gauge volume region will be taken as the integration volume. When calculating L2 (i.e., the path after the scattering all the way to detectors), still the original sample volume will be used -- I think this is how the codes are currently working but it will be nice to double check to make sure. We also need to do the same thing for the container separately, just like what we did with the sample, and the gauge volume for sure needs to be defined separately for the container to follow its shape (usually, a hollow cylinder).

All these can be done in the absorption util script for SampleOnly and SampleAndContainer types of correction. For PaalmanPingsAbsorptionCorrection, it is buried in the C++ implementation and I kind of lose myself in the code base and I may need your help on this. I will write a story for this. Tried to do that last night but the IBM tool seems to be problematic blocking me from logging in. Will do that later so we can schedule it next week or some time recently. This is a bit urgent as we do rely on a proper absorption correction for reducing samples with strong absorption quite often on both NOMAD and POWGEN. Thank you!

Kvieta1990 · 2025-02-21T21:30:16Z

@peterfpeterson could you please leave this open while I am testing to see whether the currently implementation is doing its job properly? Or should I just change to draft?

peterfpeterson

This appears to be right for simple absorption calculations. It is unclear if this would work for Paalman-Pings with the current state of the calculations.

tl;dr use for simple absorption only

peterfpeterson

Blocking for now for more testing

Kvieta1990 · 2025-03-05T04:10:44Z

Blocking for now for more testing

Hi @peterfpeterson I just tested this out, making some more changes to the code and from the test result, it seems to be working fine. For the new changes,

I put in some unit conversion internally so that users will always put in length in cm.
I fix the issue with the previous XML string format -- {:4.2F} was setting the gauge volume radius to 0 as the real value should be something like 0.00295 (with XML, it requires length in m).
For the container part of the absorption calculation, the previous implementation had some issue -- if the container gauge volume is not defined, it will use the gauge volume defined for the sample since they share the same donor workspace and the gauge volume is defined on the donor workspace only. So, if the sample gauge volume is defined, for the SampleAndContainer type of correction, we have to define the gauge volume for the container as well. I added this in with the latest commits.
I also noticed that the previous implementation missed the gauge volume definition for vanadium. I also added that in.

RichardWaiteSTFC

absorptioncorrutils is not used in the ISIS powder reduction - so I don't want to hold this up! Here are a few suggestions/questions. Also it seems like some of the PR testing instructions could be used to make a system test? However if this work is in-progress/beta I understand wanting to wait a bit until it's been validated.

Framework/PythonInterface/mantid/utils/absorptioncorrutils.py

Kvieta1990 · 2025-03-18T15:49:01Z

absorptioncorrutils is not used in the ISIS powder reduction - so I don't want to hold this up! Here are a few suggestions/questions. Also it seems like some of the PR testing instructions could be used to make a system test? However if this work is in-progress/beta I understand wanting to wait a bit until it's been validated.

I would push this part of work to another user story that the team here is working on. That work is about a once-for-all solution where the beam itself can be defined and can be used for the gauge volume definition. I think we will create some systematic system tests there. Thanks!

andy-bridger · 2025-03-19T16:06:18Z

I've been having a bit of a poke around in the AbsorptionCorrection and DefineGaugeVolume code because I was hoping to use them for an Engineering Diffraction Texture analysis workflow. I wrote up what I found 'wrong' with the code on this issue: #39079. The caveat, as I mention on there but is also discussed above here, is that if you push explicitly defining the full volume of illuminated sample onto the user, it probably works fine.

Unfortunately, as far as I am aware, the concept of gauge volume understood within texture analysis is: "the region of the object that is gauged by the instrument corresponds to a cuboid, which results from the intersection of the incident and diffracted beams" [https://doi.org/10.1107/S1600576714012710], so requiring definition of the full illuminated region would not be very intuitive. Additionally if the sample shape is complex, and imported as an STL/mesh, it might be challenging for a user to explicitly define the resultant illuminated volume.

An option I proposed to fix this is to pass both the sample shape and the gauge volume to the Rasterize function and allow the intersection of the two volumes to be handled within. I think this would fix my problem case, but a lower level change to DefineGaugeVolume, which has a flag for which region is being passed and calculates full illuminated volumes internally (basically option 2 here: #38887 (comment)) might be a preferable solution?

andy-bridger · 2025-03-19T16:16:26Z

I guess a concern is, from the AbsorptionCorrection view point, using a gauge volume defined by the full volume of illuminated sample will slow the algorithm down quite significantly. You will end up with a significantly larger bounding box for the illuminated area, which will have you rasterising this with N^3 elements, only to throw away a larger chunk of the elements not within the integration volume and throwing away a chunk more which have invalid paths?

I agree in the case of cubes, the element size could probably be set to be quite large and speed might not be an issue, but for gauge volumes with surfaces where the element sizes are required to be small issues might arise?

Also voxels are only evaluated as within volumes at their centre points, so necessitating using large elements to run in a reasonable amount of time could lead to issues there...

andy-bridger · 2025-03-19T16:41:32Z

I don't know if there is an ORNL preference for how the different interpretations of the gauge volume should be handled? @peterfpeterson @Kvieta1990

Kvieta1990 · 2025-03-19T16:45:12Z

I don't know if there is an ORNL preference for how the different interpretations of the gauge volume should be handled? @peterfpeterson @Kvieta1990

We have a user story here to be worked on where we are going to implement the beam definition and using it for defining the gauge volume. With the current PR, we are assuming the user knows what they are doing with the gauge volume definition and it is up to the users to give a proper definition of the gauge volume, i.e., the portion of the sample (and/or container) that is exposed to the beam. Technically, this is totally feasible and I have done tests with real data and it is working fine.

Kvieta1990 · 2025-03-19T16:47:07Z

@RichardWaiteSTFC I will finish the new implementation today to account for the coordinate system direction and it will be very nice if you and @peterfpeterson can help approve the PR so that we can use it (for SampleOnly and SampleAndContainer type of absorption correction). Thanks!

RichardWaiteSTFC · 2025-03-19T16:49:34Z

@RichardWaiteSTFC I will finish the new implementation today to account for the coordinate system direction and it will be very nice if you and @peterfpeterson can help approve the PR so that we can use it (for SampleOnly and SampleAndContainer type of absorption correction). Thanks!

Yep will do - thanks for the changes.
I think @andy-bridger comments relate to future planned changes we need to get the texture analysis workflow working at ISIS, rather than requests to change this PR. Perhaps it would be possible to arrange a meeting with yourself and @peterfpeterson to discuss?

Kvieta1990 · 2025-03-19T16:52:12Z

@RichardWaiteSTFC I will finish the new implementation today to account for the coordinate system direction and it will be very nice if you and @peterfpeterson can help approve the PR so that we can use it (for SampleOnly and SampleAndContainer type of absorption correction). Thanks!

Yep will do - thanks for the changes. I think @andy-bridger comments relate to future planned changes we need to get the texture analysis workflow working at ISIS, rather than requests to change this PR. Perhaps it would be possible to arrange a meeting with yourself and @peterfpeterson to discuss?

That makes a lot of sense. I will talk to Pete and will get back to you about the meeting for discussion.

Kvieta1990 · 2025-03-19T18:32:53Z

@RichardWaiteSTFC @peterfpeterson Just got the new implementation in place to cope with the potentially different coordinate system being used. I think in most cases the up direction is defined as positive-y, but it makes sense to implement the checking for direction just in case. Thanks a lot @RichardWaiteSTFC for bringing this up and your code snippet suggestion!

RichardWaiteSTFC

Thanks for the changes, looks good - sorry for delaying this when you're obviously v busy with local contacting! FYI in future I think you can also use f-strings rather than .format which is perhaps more readable - but super minor and not worth holding this up!
I'm approving, but I don't have access to the data so can't do functional testing - I'll leave that to ORNL as this change doesn't affect ISIS!

Kvieta1990 · 2025-03-20T13:38:38Z

Thanks for the changes, looks good - sorry for delaying this when you're obviously v busy with local contacting! FYI in future I think you can also use f-strings rather than .format which is perhaps more readable - but super minor and not worth holding this up! I'm approving, but I don't have access to the data so can't do functional testing - I'll leave that to ORNL as this change doesn't affect ISIS!

Thanks a lot for all your comments and sure I will be using the f-strings! Thanks!

…antidproject#38887) * enable user defined sample and container geometry for abs correction * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add in release doc * fix logic * fix beam height issue * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * remove redundant checks * fix gauge volume issue * container gauge vol definition * fix quite a few issues with the implementation * fix vanadium gauge volume def issue * add in gauge volume exception * implemented coord system checking --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: y8z <zhangy3@ornl.gov>

…- ornl-next (#39085) Enable user defined sample and container geometry for abs correction (#38887) * enable user defined sample and container geometry for abs correction * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add in release doc * fix logic * fix beam height issue * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * remove redundant checks * fix gauge volume issue * container gauge vol definition * fix quite a few issues with the implementation * fix vanadium gauge volume def issue * add in gauge volume exception * implemented coord system checking --------- Co-authored-by: Yuanpeng Zhang <zyroc1990@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: y8z <zhangy3@ornl.gov>

enable user defined sample and container geometry for abs correction

0152c97

Kvieta1990 self-assigned this Feb 19, 2025

pre-commit-ci bot and others added 3 commits February 19, 2025 04:57

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

5c9c47a

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

add in release doc

bef9c45

Merge branch 'abs_can_geo_def' of https://github.com/mantidproject/ma…

d29e357

…ntid into abs_can_geo_def

Kvieta1990 added Diffraction Issues and pull requests related to diffraction Powder Issues and pull requests related to powder diffraction Absorption Corrections Issues and pull requests related to absorption corrections labels Feb 19, 2025

fix logic

0f49dc6

sf1919 added this to the Release 6.13 milestone Feb 19, 2025

Kvieta1990 and others added 2 commits February 19, 2025 09:46

fix beam height issue

6d50042

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

f9ea5f4

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

Kvieta1990 requested a review from peterfpeterson February 19, 2025 15:10

remove redundant checks

ae5325e

peterfpeterson changed the title ~~enable user defined sample and container geometry for abs correction~~ Enable user defined sample and container geometry for abs correction Feb 20, 2025

peterfpeterson reviewed Feb 20, 2025

View reviewed changes

Framework/PythonInterface/mantid/utils/absorptioncorrutils.py Outdated Show resolved Hide resolved

y8z added 2 commits February 20, 2025 23:44

fix gauge volume issue

2daba4b

container gauge vol definition

aff3d25

peterfpeterson approved these changes Feb 21, 2025

View reviewed changes

peterfpeterson requested changes Feb 24, 2025

View reviewed changes

y8z added 2 commits March 4, 2025 22:24

fix quite a few issues with the implementation

dc87c18

fix vanadium gauge volume def issue

4caafff

Kvieta1990 requested review from peterfpeterson and RichardWaiteSTFC March 5, 2025 04:11

peterfpeterson previously approved these changes Mar 11, 2025

View reviewed changes

RichardWaiteSTFC requested changes Mar 12, 2025

View reviewed changes

Framework/PythonInterface/mantid/utils/absorptioncorrutils.py Outdated Show resolved Hide resolved

Framework/PythonInterface/mantid/utils/absorptioncorrutils.py Outdated Show resolved Hide resolved

Framework/PythonInterface/mantid/utils/absorptioncorrutils.py Show resolved Hide resolved

add in gauge volume exception

182d300

Kvieta1990 dismissed peterfpeterson’s stale review via 182d300 March 18, 2025 15:45

implemented coord system checking

728795b

RichardWaiteSTFC approved these changes Mar 20, 2025

View reviewed changes

peterfpeterson merged commit 5e41da9 into main Mar 20, 2025
10 checks passed

peterfpeterson deleted the abs_can_geo_def branch March 20, 2025 13:43

peterfpeterson mentioned this pull request Mar 20, 2025

Enable user defined sample and container geometry for abs correction - ornl-next #39085

Merged

Enable user defined sample and container geometry for abs correction #38887

Enable user defined sample and container geometry for abs correction #38887

Uh oh!

Conversation

Kvieta1990 commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of work

Summary of work

To test:

This is the one for testing the gauge volume definition

Here is the one for testing the geometry definition,

Here is another one for testing the geometry definition, with only beam height defined (and therefore the gauge volume will be defined internally in Mantid by assuming the beam location),

Reviewer

Code Review

Functional Tests

Gatekeeper

Uh oh!

Uh oh!

Kvieta1990 commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kvieta1990 commented Feb 21, 2025

Uh oh!

peterfpeterson left a comment

Choose a reason for hiding this comment

Uh oh!

peterfpeterson left a comment

Choose a reason for hiding this comment

Uh oh!

Kvieta1990 commented Mar 5, 2025

Uh oh!

RichardWaiteSTFC left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kvieta1990 commented Mar 18, 2025

Uh oh!

andy-bridger commented Mar 19, 2025

Uh oh!

andy-bridger commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andy-bridger commented Mar 19, 2025

Uh oh!

Kvieta1990 commented Mar 19, 2025

Uh oh!

Kvieta1990 commented Mar 19, 2025

Uh oh!

RichardWaiteSTFC commented Mar 19, 2025

Uh oh!

Kvieta1990 commented Mar 19, 2025

Uh oh!

Kvieta1990 commented Mar 19, 2025

Uh oh!

RichardWaiteSTFC left a comment

Choose a reason for hiding this comment

Uh oh!

Kvieta1990 commented Mar 20, 2025

Uh oh!

Uh oh!

Uh oh!

Kvieta1990 commented Feb 19, 2025 •

edited

Loading

Kvieta1990 commented Feb 21, 2025 •

edited

Loading

andy-bridger commented Mar 19, 2025 •

edited

Loading