-
Notifications
You must be signed in to change notification settings - Fork 128
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Enable user defined sample and container geometry for abs correction #38887
base: main
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
…ntid into abs_can_geo_def
for more information, see https://pre-commit.ci
) | ||
|
||
if beam_height != Property.EMPTY_DBL and not gauge_vol: | ||
gauge_vol = """<cuboid id="shape"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is strange in a way similar to the DefineGaugeVolume user docs. I think it is configured that
- front-back is x-dimension
- left/right is y-dimension (beam height)
- top/bottom is z-dimension
It would be good to also add in the beam width for the dimension that has that
Adding a comment explaining that will help future people who see this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DefineGaugeVolume user docs is indeed a bit confusing in the explanation of how the gauge volume is defined. Here are several comments,
-
For the cuboid volume definition, I created an image to demonstrate the idea,
-
Ideally, indeed the gauge volume could be defined as above a a cuboid and Mantid then worries about constructing the intersection area between the sample and the defined gauge volume. However, the construction of the intersection area is not implemented yet. So, we cannot go with this route.
-
However, the gauge volume can be defined as with the same shape as the sample, e.g., cylinder or hollow cylinder and current implementation can meet our need without problems. We do need some further implementation in this PR to define the gauge volume for the sample and container separately.
-
The implementation for
SampleOnly
andSampleAndContainer
method is straightforward -- just define the gauge volume twice here -
For the
PaalmanPingsAbsorptionCorrection
method, I am not sure how to do that -- the same principle applies but I am not so familiar with the CPP codes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At ISIS we are also interested in starting to use DefineGaugeVolume
with other attenuation algorithms such as MonteCarloAbsoption
for use on our engineering beamline ENGIN-X. For us we would need to consider the case of when the gauge volume is not wholly occupied by the sample, both for attenuation correction and normalising by the illuminated sample volume. If you do implement this let us know! Not sure how this fits in with your plans? Happy to discuss!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure. I will keep you updated about the implementation. We do plan to do the gauge volume definition for both the absorption correction and normalization purpose. Currently I am working on this and the PR here is part of the work. Maybe we need another PR for working with implementation for PaalmanPingsAbsorptionCorrection
which may need the help from @peterfpeterson and his team since I am not so familiar the C++ code and the code architecture there^_^ Anyhow, I will keep you in the loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is also the algorithm SetBeam
which may be appropriate for these calculations. I'm still in progress on understanding the effect of it.
@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 |
@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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 toSetSample
. 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
Here is the one for testing the geometry definition,
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
Functional Tests
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.