BUG: Fix minor errors in mayo geometry definitions #1383

adler-j · 2018-06-20T20:39:19Z

It seems we had some minor (but notable in some cases) errors with the geometry descriptions.

adler-j · 2018-06-20T21:20:33Z

odl/contrib/datasets/ct/mayo.py


-    # TODO: WE CURRENTLY IGNORE THE OFFSETS DUE TO FLYING FOCAL SPOT
-    source_offsets = np.array([offset_x, offset_y, offset_z]).T
+    # Make a parallel beam geometry with flat detector


Wrong comment

adler-j · 2018-06-20T21:20:38Z

odl/contrib/datasets/ct/mayo.py

+        src_radius = src_radius + np.mean(offset_radial)
+        angles = angles - np.mean(offset_angular)
+
+        # print(np.mean(offset_angular))


Remove print

kohr-h · 2018-06-21T07:37:17Z

I guess this isn't quite ready for review yet

adler-j · 2018-06-21T14:29:43Z

Its reviewable, just some minor stuff to fix.

kohr-h

I'm not too familiar with the conventions, so I can't say anything about correctness. But still one thing I should understand but don't.

kohr-h · 2018-06-21T14:45:38Z

odl/contrib/datasets/ct/mayo.py

-
-    # TODO: WE CURRENTLY IGNORE THE OFFSETS DUE TO FLYING FOCAL SPOT
-    source_offsets = np.array([offset_x, offset_y, offset_z]).T
+    if 1:


I guess this is for development only?
If not, I'm not in favor of "parking" backup code like this. Better make an issue and post the old code there for reference. It's also in the git history anyway.

I'll remove it.

kohr-h · 2018-06-21T14:49:57Z

odl/contrib/datasets/ct/mayo.py

-    source_offsets = np.array([offset_x, offset_y, offset_z]).T
+    if 1:
+        # Apply only mean of offsets
+        src_radius = src_radius + np.mean(offset_radial)


I don't get this. How could this ever work before if it's a vector? In the old version, src_rad_offset must have been a vector, and with it all 3 offsets as well.

Basically the offsets are weird, so I decided to just go for their means. Before we did not do anything about it (src_rad_offset was unused) which gave some slight errors, since apparently the "offsets" are not zero-centered.

Okay. What I was puzzled about was that in the old version, src_radius became a vector (after adding offset_radial), then it was used to compute offset_[xyz], each of which also was a vector then, and the final source_offsets stacked them to a matrix. I don't see how that made sense.

Anyhow, for some other PR.

adler-j · 2018-06-25T13:44:36Z

With some further small fixes, it now actually looks like this does the right thing, and there are no magic numbers!

A note to future readers (e.g myself in a year): The tags "SpiralPitchFactor" and "TableFeedPerRotation" are slightly bugged and not to be trusted.

adler-j · 2018-06-28T06:28:11Z

Could use a final review on this

aringh

Looks good to me, but honestly I am not too familiar with the dicom format and with the mayo-data.

aringh · 2018-06-28T08:00:12Z

odl/contrib/datasets/ct/mayo.py

-        data_array /= 1024.0
+        # Convert from storage type to densities
+        # TODO: Optimize these computations
+        hu_values = (dataset.RescaleSlope * data_array +


Now I will reveal my lack of knowledge in this field: what is the difference between this hu_values and the hu_factor on line 57?

hu_values is an array (basically the image) while hu_factor is an integer specifying how to rescale values.

Basically DICOM stores everything as positive integers, and then has information on how to convert this into usable stuff by rescaling. That's what I do here. I also follow up by moving from HU values to densities (of water) since the values are much easier to work with later on.

kohr-h

Some minor comments, but largely good to go.

As before, I assume the code is correct. It does make sense on a high level.

kohr-h · 2018-06-28T07:32:46Z

odl/contrib/datasets/ct/mayo.py

+        src_radius / (src_radius + det_radius))
+
+    # Convert offset to odl defintions
+    offset_along_axis = (offset_along_axis +


Why not +=?

I want to keep this as a "unified" assignment. I'd actually prefer to replace the "old" offset_along_axis to something like "mean_offset_along_axis_for_ffz". It's also more honest.

kohr-h · 2018-06-28T08:12:18Z

odl/contrib/datasets/ct/mayo.py

+    if len(datasets) > 1:
+        slice_distance = np.abs(
+            np.array(datasets[1].DataCollectionCenterPatient)[2] -
+            np.array(datasets[0].DataCollectionCenterPatient)[2])


np.array shouldn't be necessary?

DataCollectionCenterPatient[2] is a string.

Although I guess float would make more sense.

Yes, definitely. I thought it was a list.

kohr-h · 2018-06-28T08:15:12Z

odl/contrib/datasets/ct/mayo.py

+    min_pt[2] -= 0.5 * slice_distance
    max_pt[2] = -np.array(datasets[-1].DataCollectionCenterPatient)[2]
-    max_pt[2] += 0.5 * pixel_thickness
+    max_pt[2] += 0.5 * slice_distance


What's the overall logic of computing the min/max in the z axis? Does it go from -(patient z in dataset 0) to 0, adjusted by half a slice distance in each direction?

Okay, maybe a comment could make that more obvious, I had to think about it for a second. Maybe also the review markup was a bit to blame.

kohr-h · 2018-06-28T08:17:25Z

odl/contrib/datasets/ct/mayo.py

+    # Apply only mean of offsets
+    src_radius = src_radius + np.mean(offset_radial)
+    offset_along_axis = np.mean(offset_axial) * (
+        src_radius / (src_radius + det_radius))


Comment on why the multiplication with the magnification factor? (Which BTW would be a good name for a temp variable, to clarify things a bit.)

I'll try to write this down, but overall this really needs a whiteboard

kohr-h · 2018-06-28T08:17:50Z

odl/contrib/datasets/ct/mayo.py

-    offset_y = (np.sin(angles_offset) * (-src_rad_offset) -
-                np.sin(angles) * (-src_radius))
-    offset_z = offset_axial
+    # Correct the angular sampling according to corrections


This doesn't really say anything

kohr-h · 2018-06-28T08:18:08Z

odl/contrib/datasets/ct/mayo.py

-                         angles[0] / (2 * np.pi) * pitch)
+    # For unknown reasons, mayo does not include the tag
+    # "TableFeedPerRotation", which is what we want.
+    # Instead we manually compute the pitch


This is a great comment!

kohr-h · 2018-06-28T08:19:16Z

odl/contrib/datasets/ct/mayo.py


-    # TODO: WE CURRENTLY IGNORE THE OFFSETS DUE TO FLYING FOCAL SPOT
-    source_offsets = np.array([offset_x, offset_y, offset_z]).T
+    # TODO: Implement proper handling of flying focal spot


How about you write # TODO(adler-j)? Then people looking at this know whom to ask without doing a git blame.

adler-j · 2018-06-28T08:41:29Z

Fixed comments, merge after CI.

adler-j commented Jun 20, 2018

View reviewed changes

adler-j force-pushed the mayo_improvements branch from 219f485 to bd7cb07 Compare June 21, 2018 14:33

kohr-h reviewed Jun 21, 2018

View reviewed changes

adler-j added 2 commits June 25, 2018 10:32

BUG: Fix minor errors in mayo geometry definitions

4d68bc3

ENH: Remove leftover code in mayo.load_projections

5098fad

adler-j force-pushed the mayo_improvements branch from 0cda683 to 5098fad Compare June 25, 2018 08:32

ENH: Further minor geometry fixes to mayo

0d3f164

ENH: Minor fix to density computation for mayo

fb87892

aringh reviewed Jun 28, 2018

View reviewed changes

kohr-h approved these changes Jun 28, 2018

View reviewed changes

DOC: Add more explanatory comments on mayo geometries

6c670c4

adler-j merged commit 6a18fb0 into odlgroup:master Jun 28, 2018

adler-j deleted the mayo_improvements branch June 28, 2018 09:10

BUG: Fix minor errors in mayo geometry definitions #1383

BUG: Fix minor errors in mayo geometry definitions #1383

Uh oh!

Conversation

adler-j commented Jun 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kohr-h commented Jun 21, 2018

Uh oh!

adler-j commented Jun 21, 2018

Uh oh!

kohr-h left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adler-j commented Jun 25, 2018

Uh oh!

adler-j commented Jun 28, 2018

Uh oh!

aringh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kohr-h left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adler-j commented Jun 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!