Conversation
|
Could this get a review please? |
|
I'll have a look |
kohr-h
left a comment
kohr-h
left a comment
There was a problem hiding this comment.
Couple of things, please check.
Code reuse is quite low in the module. For instance, the cuboid_phantom function seems to have about 95 % overlap with ellipsoid_phantom (not counting the docstring). And major parts of _cuboid_phantom_2d are copy-pasted from the ellipse case as well.
If you see a possibility, try to factor out the parts that allow it. Otherwise, please add a TODO comment.
| else: | ||
| # Calculate the points that could possibly be inside the volume | ||
| max_radius = np.sqrt([a_squared, b_squared]) | ||
| idx, shapes = _getshapes_2d(center, max_radius, space.shape) |
There was a problem hiding this comment.
Why do you have to do this for non-rotated rectangles? Can't you just get the min and max per axis and use those directly as bounding boxes? In other words, can't you directly construct slice objects from the min and max and use those for indexing?
There was a problem hiding this comment.
OTOH, I can see why you want to use the same kind of logic. Maybe it's okay.
There was a problem hiding this comment.
I'll just leave this as is since the other optimization is relatively minor for most cases. This would only give a notable improvement in the case of slightly rotated and very elongated rectangles, which I'll just leave for now.
odl/phantom/geometric.py
Outdated
| # Parentheses to get best order for broadcasting | ||
| max_distance = np.maximum(squared_dist[0], squared_dist[1]) | ||
|
|
||
| # Find the points within the ellipse |
odl/phantom/geometric.py
Outdated
| # Find the points within the ellipse | ||
| inside = max_distance <= 1 | ||
|
|
||
| # Add the ellipse intensity to those points |
|
|
||
| The provided cuboids need to be specified relative to the | ||
| reference rectangle ``[-1, -1] x [1, 1]``. | ||
| The angles are to be given in radians. |
There was a problem hiding this comment.
3D is not currently supported, I'll update the rest to reflect this
odl/phantom/geometric.py
Outdated
| The phantom is created by adding the values of each cuboid. The | ||
| cuboid are defined by a center point | ||
| ``(center_x, center_y, [center_z])``, the lengths of its principial | ||
| axes ``(axis_1, axis_2, [axis_2])``, and a rotation angle ``rotation`` |
There was a problem hiding this comment.
But since 3D isn't supported, remove 3rd axis stuff.
odl/phantom/geometric.py
Outdated
| [ 0., 1., 2., 1., 0.], | ||
| [ 1., 2., 2., 2., 1.], | ||
| [ 0., 1., 2., 1., 0.], | ||
| [ 0., 0., 1., 0., 0.]] |
There was a problem hiding this comment.
Something is not right with this example. It doesn't look like a cuboid to me.
odl/phantom/geometric.py
Outdated
| Space in which the phantom should be created, must be 2- or | ||
| 3-dimensional. If ``space.shape`` is 1 in an axis, a corresponding | ||
| slice of the phantom is created (instead of squashing the whole | ||
| phantom into the slice). |
There was a problem hiding this comment.
3D is not supported, so this needs an update
odl/phantom/geometric.py
Outdated
| space, min_pt=snapped_min_pt, max_pt=snapped_max_pt, | ||
| cell_sides=space.cell_sides) | ||
|
|
||
| tmp_phantom = _phantom(tmp_space, ellipsoids) |
There was a problem hiding this comment.
This branch has obviously never been run, since it cannot work.
|
Checking updated PR...
|
kohr-h
left a comment
kohr-h
left a comment
There was a problem hiding this comment.
Minor thing to fix, after that I guess we can merge.
| space = odl.uniform_discr([-1, -1], [1, 1], [300, 300]) | ||
| ellipses = [[1.0, 0.6, 0.6, 0.0, 0.0, 1.5], | ||
| [1.0, 0.1, 0.3, 0.0, 0.0, 0.3]] | ||
| cuboid_phantom(space, ellipses).show('cuboid phantom 2d') |
There was a problem hiding this comment.
Use cuboids or cubes instead of ellipses.
3 participants
No description provided.