Skip to content
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

Make calculation conform to figure in https://arxiv.org/abs/2403.08538 #17

Merged
merged 6 commits into from
Apr 17, 2024

Conversation

uellue
Copy link
Member

@uellue uellue commented Apr 15, 2024

Previously, the camera length was measured from the focus point, not the specimen plane as written in the paper

Previously, the camera length was measured from the focus point,
not the specimen plane as written in the paper
Copy link

codecov bot commented Apr 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@814fa7b). Click here to learn what that means.

Additional details and impacted files
@@           Coverage Diff            @@
##             main       #17   +/-   ##
========================================
  Coverage        ?   100.00%           
========================================
  Files           ?         5           
  Lines           ?       190           
  Branches        ?        21           
========================================
  Hits            ?       190           
  Misses          ?         0           
  Partials        ?         0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@uellue uellue requested a review from sk1p April 15, 2024 16:43
Copy link
Member

@sk1p sk1p left a comment

Choose a reason for hiding this comment

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

I've tried to understand this, I hope my comments are helpful

tests/test_overfocus.py Show resolved Hide resolved
tests/test_overfocus.py Outdated Show resolved Hide resolved
tests/test_overfocus.py Outdated Show resolved Hide resolved
tests/test_overfocus.py Outdated Show resolved Hide resolved
tests/test_overfocus.py Outdated Show resolved Hide resolved
Thx! :-)
@uellue uellue requested a review from sk1p April 16, 2024 09:21
Comment on lines 84 to 85
y_px, x_px : float
Detector pixel coordinates to project
Copy link
Member

Choose a reason for hiding this comment

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

As these are float, does (0.0, 0.0) correspond to the center of the top-left pixel, or to the top-left corner of the top-left pixel?

Copy link
Member

Choose a reason for hiding this comment

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

Or is (0, 0) actually the "central pixel" of the detector, which is then actually the center of that pixel? That's what I gather from the "straight through" test case below...

Comment on lines 88 to 95
detector_pixel_size, scan_pixel_size : float
Physical pixel sizes. This assumes a uniform scan and detector grid in x
and y direction
camera_length : float
Virtual distance from specimen to detector
overfocus : float
Virtual distance from focus point to specimen. Underfocus is specified
as a negative overfocus.
Copy link
Member

Choose a reason for hiding this comment

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

What's the unit of these - probably all of these just have to be given in the same unit, but units don't matter otherwise?

LiberTEM. It can be calculated with :fun:`get_transformation_matrix`.
fov_size_y, fov_size_x : float
Size of the scan area (field of view) in scan pixels. The scan
coordinate system is centered in the middle of the field of view.
Copy link
Member

Choose a reason for hiding this comment

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

What exactly does "centered" mean here? Where does the scan coordinate system have its origin?

y_px, x_px : float
Detector pixel coordinates to project
cy, cx : float
Detector center in detector pixel coordinates
Copy link
Member

Choose a reason for hiding this comment

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

What does "center" mean here? (h/2, w/2)? Or the pixel which the optical axis is contained in?

Comment on lines +64 to +65
'fov_size_y': 0,
'fov_size_x': 0,
Copy link
Member

Choose a reason for hiding this comment

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

What does it mean to have fov_size_{x,y}=0? Shouldn't the scan have at least one scan pixel?

detector_shape=(detector_size, detector_size),
sim_params=params,
)
# Center of the scan, severy second detector pixel
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Center of the scan, severy second detector pixel
# Center of the scan, every second detector pixel

tests/test_overfocus.py Show resolved Hide resolved
Copy link
Member

@sk1p sk1p left a comment

Choose a reason for hiding this comment

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

Thanks for the updates - the doc string and comments make it a lot more clear! I've added some more questions - I think some of them can be answered by looking at the figure from the paper, on the other hand, with a bit more clear language it can stand on its own.

@uellue uellue requested a review from sk1p April 16, 2024 13:14
@sk1p sk1p merged commit 5bbebcf into LiberTEM:main Apr 17, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants