-
Notifications
You must be signed in to change notification settings - Fork 105
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
WIP: Add vector geometries #1418
base: master
Are you sure you want to change the base?
Conversation
angle_partition, detector_partition, src_radius=1000, det_radius=100, | ||
axis=[1, 0, 0]) | ||
|
||
circle_vecs = odl.tomo.astra_conebeam_3d_geom_to_vec(circle_geometry) |
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.
I've renamed this function, TODO change here
odl/discr/grid.py
Outdated
@@ -54,6 +54,7 @@ def sparse_meshgrid(*x): | |||
xi = np.asarray(xi) | |||
slc = [None] * n | |||
slc[ax] = slice(None) | |||
slc = tuple(slc) |
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.
Code like this without conversion from list to tuple raises a DeprecationWarning
in NumPy 1.15, we should apply the fix throughout.
'DetectorCount': 10, 'DetectorWidth': 0.2} | ||
|
||
assert is_subdict(correct_subdict, proj_geom) | ||
assert 'ProjectionAngles' in proj_geom |
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 test was kind of pointless since it really tests ASTRA rather than ODL, and some implementation detail of ODL. Not sure if we really need a replacement.
odl/tomo/backends/astra_cuda.py
Outdated
elif geometry.ndim == 3: | ||
det_px_area = geometry.det_partition.cell_volume | ||
scaling_factor *= (det_px_area ** 2 / | ||
reco_space.cell_volume ** 2) |
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.
Some testing needed here
odl/tomo/backends/astra_setup.py
Outdated
# ODL x = ASTRA -y, ODL y = ASTRA x | ||
# Thus: ODL->ASTRA conversion is a rotation by +90 degrees | ||
rot_90 = euler_matrix(np.pi / 2) | ||
# Bulk matrix-vector product |
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.
with the transposed matrix
odl/tomo/backends/astra_setup.py
Outdated
if vecs.shape[1] == 6: | ||
# 2D geometry | ||
# ODL x = ASTRA -y, ODL y = ASTRA x | ||
# Thus: ODL->ASTRA conversion is a rotation by +90 degrees |
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.
Maybe these comments are redundant
# TODO: shapes are not quite right here. Scalar works, but | ||
# vectorized goes wrong | ||
ray_dir = np.empty(bcast_shape + (self.ndim,), dtype=float) | ||
print('result shape:', ray_dir.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.
Debug statement, TODO remove
Regarding vectorization, I'd suggest putting it in a separate PR.
Should this be reviewed? |
Hm, I think some things are still a bit in the flow, so I'll make it a WIP in the title. I'll give a heads-up when it's ready. |
621e03a
to
8d211d6
Compare
Checking updated PR...
Comment last updated at 2020-11-08 00:20:59 UTC |
f6bf263
to
4052454
Compare
4052454
to
521d7cf
Compare
Deferring until #1540 has landed. Too many overlapping changes. |
Okay, time to pick this one up again. |
I did a merge with a few minor fixes, may or may not be useful. See your repo. |
Thanks, I saw it. I'll take it as reference. 👍 |
1ca6fed
to
252b7d0
Compare
odl/tomo/backends/astra_cuda.py
Outdated
elif isinstance(geometry, VecGeometry): | ||
scaling_factor = (geometry.det_partition.cell_volume / | ||
vol_space.cell_volume) | ||
elif isinstance(geometry, ParallelVecGeometry): | ||
if geometry.ndim == 2: | ||
# Scales with 1 / cell_volume | ||
scaling_factor *= float(vol_space.cell_volume) | ||
elif geometry.ndim == 3: | ||
# Scales with cell volume | ||
scaling_factor /= vol_space.cell_volume |
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.
Some part of this logic makes no sense...
I picked up an old WIP branch that I currently need. The code adds support for ASTRA vec geometries in a fairly direct way.
Since the coordinate systems of ODL and ASTRA differ, I've taken the following approach:
ParallelVecGeometry
andConeVecGeometry
take a matrix of shape(N, 6)
for 2D or(N, 12)
for 3D and determinendim
accordingly.(rayX, rayY)
is defined with respect to the ODL coordinate system.As a consequence, a conversion step is necessary before handing these vectors over to ASTRA (or, inversely, when we get vectors from ASTRA or from somewhere with ASTRA coordinates).
The reasoning behind this is that ODL should use consistent coordinates internally and translate to other systems when interfacing with other libraries. The "list of vectors" format is just treated as a representation for geometries, without any prescribed coordinate system.
I've added a couple of "check" examples to make sure that the transformations are defined correctly. It's looking good, and the existing geometries are still correct.
TODO:
*3d_geom_to_vecs
returns the ASTRA vectors "as expected", and downstream code does the swapping instead.Clean up commitsCloses #1023