-
Notifications
You must be signed in to change notification settings - Fork 41
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.
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
Phase contribution #1737
Phase contribution #1737
Conversation
Signed-off-by: Hannah Robarts <[email protected]>
We don't normally require dask do we? Can we avoid it?
…On Mon, 4 Mar 2024 at 15:58, Casper da Costa-Luis ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Wrappers/Python/cil/processors/PaganinPhaseProcessor.py
<#1737 (comment)>
:
> @@ -0,0 +1,387 @@
+# -*- coding: utf-8 -*-
⬇️ Suggested change
-# -*- coding: utf-8 -*-
------------------------------
In Wrappers/Python/cil/processors/PaganinPhaseProcessor.py
<#1737 (comment)>
:
> +import dask
+from dask import delayed
if dask is a new dependency you'll need to add it to:
- recipe/meta.yaml
- scripts/create_local_env_for_cil_development.sh
- scripts/requirements-test.yml
—
Reply to this email directly, view it on GitHub
<#1737 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACMDSCBUL3QNPDBDJ625ERTYWSD2BAVCNFSM6AAAAABECCP67KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMJUGUZTMNBYGI>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Hi Jakob, yes we can avoid it. I thought it already was a dependency but I can remove it if not |
Signed-off-by: Hannah Robarts <[email protected]>
…L into phase_contribution
…L into phase_contribution
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.
It's looking good. Now I've had a good look I think we need to update the units handling here as I've suggested in individual comments. We can discuss this more though.
Otherwise I see some memory issues with the padding, but that's an easy fix.
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.
Hi @hrobarts thanks for the update, I am happy with the changes. I added a couple of minor points and also a couple of my previous conversations remain open, could you take a look and respond to each so we can see (and have a record of) what has been done and if can be resolved? Thanks!
Co-authored-by: Jakob Sauer Jørgensen <[email protected]> Signed-off-by: Hannah Robarts <[email protected]>
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.
Happy with it!
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.
It looks great! I'm happy once tests pass.
Changes
Add Paganin phase retrieval methods based on the description in https://onlinelibrary.wiley.com/doi/10.1046/j.1365-2818.2002.01010.x
The phase retrieval can be applied using
Runs the Paganin method, returning the sample thickness,
$ T = -\frac{1}{\mu}\ln(F^{-1}\frac{F(M^2 I_{norm}(x,y,z=\Delta))}{1+\frac{\Delta\lambda\delta}{4\pi\beta}(k_x^2+k_y^2)})$
Or with the
$ T = -\frac{1}{\mu}\ln(F^{-1}\frac{F(M^2 I_{norm}(x,y,z=\Delta))}{1-\frac{2\Delta\lambda\delta}{4\pi\beta W^2}(cos(Wk_x)+cos(Wk_y)-2)})$
filter_type
argument='generalised_paganin_method'
uses the filter described in https://iopscience.iop.org/article/10.1088/2040-8986/abbab9Both methods require normalised transmission data,$I_{norm}$ , physical parameters for delta, beta (the real and complex part of the material refractive index) and energy. The propagation distance, $\Delta$ , magnification, $M$ , and pixel size, $W$ , are accessed from the data geometry. If these values are not found in the geometry, the processor prints a warning and uses default values. The geometry values can be over-ridden using
We can return only the filtered image using,
which returns
Testing you performed
I've made a pull request to add an example to CIL-Demos/misc using these methods
TomographicImaging/CIL-Demos#151
Below is an output from the notebook showing a comparison of the cross-section of a test image with different phase retrieval methods. The scaled phase retrieval in CIL matches the phase retrieval method in Tomopy when performed on transmission data then converted to absorption, and the filter in CIL matches the Tomopy method performed on absorption data
where the Tomopy regularisation parameter argument is
tomopy_alpha = 1/(4*np.pi**2*(delta/beta))
Unit tests of the functionality are in test_DataProcessor.py
TestPaganinPhaseRetriver()
andTestFilter()
Related issues/links
Checklist
Contribution Notes
Please read and adhere to the developer guide and local patterns and conventions.