8000 Update ESRF demo by hrobarts · Pull Request #224 · TomographicImaging/CIL-Demos · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Update ESRF demo #224

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Feb 6, 2025
Merged

Update ESRF demo #224

merged 9 commits into from
Feb 6, 2025

Conversation

hrobarts
Copy link
Contributor
@hrobarts hrobarts commented Jan 28, 2025 •
edited
Loading

New ESRF demo notebook using TomoBank data

Closes #237

@hrobarts hrobarts self-assigned this Jan 28, 2025
lauramurgatroyd
lauramurgatroyd requested changes Jan 29, 2025
View reviewed changes
Copy link
Member
@lauramurgatroyd lauramurgatroyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good and ran fine with the latest cil. Just have some suggestions for improvements

lauramurgatroyd
lauramurgatroyd requested changes Jan 31, 2025
View reviewed changes
Copy link
Member
@lauramurgatroyd lauramurgatroyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great with the extra docs info

@hrobarts
Copy link
Contributor Author

Thank you for reviewing @lauramurgatroyd, these should all be updated now

@hrobarts hrobarts requested a review from gfardell January 31, 2025 16:05
lauramurgatroyd
lauramurgatroyd approved these changes Jan 31, 2025
View reviewed changes
Copy link
Member
@lauramurgatroyd lauramurgatroyd left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only comment is that phase retrieval makes the start and end slices weird

Without it:
image

with it:

image

@lauramurgatroyd
Copy link
Member

I like the new text surrounding the phase but I am not sure about the claim: "The sample feature (at horizontal_x, horizontal_y=35 in the plot above) is reduced in intensity and blurred but we find the SNR of the feature is improved. "

It looks like what you have labelled as "Sample feature SNR" is a SNR of the full line profile, not just focussing on this feature?

@hrobarts
Copy link
Contributor Author
hrobarts commented Feb 5, 2025

I like the new text surrounding the phase but I am not sure about the claim: "The sample feature (at horizontal_x, horizontal_y=35 in the plot above) is reduced in intensity and blurred but we find the SNR of the feature is improved. "

It looks like what you have labelled as "Sample feature SNR" is a SNR of the full line profile, not just focussing on this feature?

Hi Laura, I have updated the SNR calculation to include just the feature and some background (we need the background to get the noise!)
image
Sample feature SNR = 6.334
Sample feature SNR after phase retrieval = 19.542

@lauramurgatroyd
Copy link
Member

Looks good!

@gfardell
Copy link
Member
gfardell commented Feb 5, 2025

Sorry, all in one as it's difficult to comment on a notebook directly.

  1. I don’t understand the point of making a copy of the data, the processor won't change it. And you still have some confusion if you want to rerun it on the original input. To me it is more flexible, and more clear to use:

processor.set_input(data_before)
data = processor.get_output()

  1. The text: "Also look at a vertical slice of the data which will allow us to compare the effect on the sinograms"
    I think this would be clearer to say something like "Look at the sinogram for a detector row, here we select vertical index=420"

  2. The prints:

print("Centre of rotation before {}, rotation axis position {}"
      .format(data_before.geometry.get_centre_of_rotation(distance_units='pixels'), data_before.geometry.config.system.rotation_axis.position))
print("Centre of rotation after {}, rotation axis position {}"
      .format(data.geometry.get_centre_of_rotation(distance_units='pixels'), data.geometry.config.system.rotation_axis.position))

I don't really see the point of showing the rotation axis position some calls might shift and reorientate the coordinate system. the CoR could be encoded there, or in the detector position or partially in both, so depending on the state of your data taking this in isolation is meaningless whereas 'get_centre_of_rotation' takes this in to account.

  1. Just for fun, the difference between the image_sharpness and the cross-correlation methods (although it’s clearer when you toggle between them directly). Image_sharpness has probably found a closer value, they differ by 0.25 of a pixel, but image_sharpness is a slightly higher contrast and doesn’t introduce the ring artefact. If you have 360 degrees of data why not use it all rather than 2 projections?

image
image

The other issue could be that this is a tall sample, so the offset might change in different slices.

  1. You have a random unused code cell
    title = tuple(['No ring remover'] + ['Ring remover decNum: ' + str(p) + ', index: ' for p in decNum_list])

  2. I would suggest when showing how things have worked you zoom in on a smaller ROI. Things look nice when your display has downsampled them. Maybe 2 plots of the full object, and 2 of feature - it canbe a different ROI depending what you want to demonstrate.

  3. On paganin with:
    delta = 3e-5
    beta = 2e-10

image

image

Wouldn’t these be the better values? It’s removed the fringes completely and acted like a blurring filter (as expected).

I think it’s also important to note these aren’t clear features, and we’re not tuning it around them. With the stronger paganin we can still find slices with clear features - don’t be afraid to change the contrast as well. Maybe show2D will help fix a particular view, but encourage them to explore their data.

image

@hrobarts
Copy link
Contributor Author
hrobarts commented Feb 6, 2025

Thank @gfardell these are all really good points. I've updated the notebook

@hrobarts hrobarts merged commit da07f77 into main Feb 6, 2025
@hrobarts hrobarts deleted the esrf_demo branch February 6, 2025 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lauramurgatroyd lauramurgatroyd lauramurgatroyd approved these changes

@gfardell gfardell Awaiting requested review from gfardell

Assignees

@hrobarts hrobarts

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ESRF demo notebook for training
3 participants
@hrobarts @lauramurgatroyd @gfardell
0