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

Improve X-ray docs #578

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 32 additions & 13 deletions scico/linop/xray/astra.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,18 @@ def _project_coords(
x_volume: np.ndarray, vol_geom: VolumeGeometry, proj_geom: ProjectionGeometry
) -> np.ndarray:
"""
Transform volume (logical) coordinates into world coordinates based
Project volume (logical) coordinates into detector coordinates based
Copy link
Collaborator

@bwohlberg bwohlberg Feb 11, 2025

Choose a reason for hiding this comment

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

Is this a change of terminology from "world coordinates" to "detector coordinates"? If so, corresponding changes are required elsewhere for consistency.

Why are volume coordinates "logical"?

on ASTRA geometry objects.

Args:
x_volume: (..., 3) vector(s) of volume (AKA logical) coordinates
Copy link
Collaborator

Choose a reason for hiding this comment

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

AKA is a bit jarring. Perhaps "i.e."?

vol_geom: ASTRA volume geometry object.
proj_geom: ASTRA projection geometry object.

Returns:
(num_angles, ..., 3) array of detector coordinates corresponding
to projections of the points in `x_volume`.

"""
det_shape = (proj_geom["DetectorRowCount"], proj_geom["DetectorColCount"])
x_world = volume_coords_to_world_coords(x_volume, vol_geom=vol_geom)
Expand All @@ -95,7 +100,10 @@ def project_world_coordinates(
"""Project world coordinates along ray into the specified basis.

Project world coordinates along `ray` into the basis described by `u`
and `v` with center `d`.
and `v` with center `d`. The term ""world"" emphasizes that the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why double quotes on either side of "world"?

function is intended to be used on 3D coordinates representing a
point in physical space, rather than a logical index into the volume
Copy link
Collaborator

Choose a reason for hiding this comment

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

"logical" implies an integer index into an array?

or detector arrays.

Args:
x: (..., 3) vector(s) of world coordinates.
Expand Down Expand Up @@ -317,6 +325,9 @@ def fbp(self, sino: jax.Array, filter_type: str = "Ram-Lak") -> jax.Array:
filter_type: Select the filter to use. For a list of options
see `cfg.FilterType` in the `ASTRA documentation
<https://www.astra-toolbox.com/docs/algs/FBP_CUDA.html>`__.

Returns:
Reconstructed volume.
"""

def f(sino):
Expand Down Expand Up @@ -414,7 +425,7 @@ def _astra_to_scico_geometry(vol_geom: VolumeGeometry, proj_geom: ProjectionGeom

"""
x_volume = np.concatenate((np.zeros((1, 3)), np.eye(3)), axis=0) # (4, 3)
x_dets = _project_coords(x_volume, vol_geom, proj_geom) # (1, 4, 2)
x_dets = _project_coords(x_volume, vol_geom, proj_geom) # (num_angles, 4, 2)

x_volume_aug = np.concatenate((x_volume, np.ones((4, 1))), axis=1) # (4, 4)
matrices = []
Expand All @@ -433,10 +444,7 @@ def convert_to_scico_geometry(
angles: Optional[np.ndarray] = None,
vectors: Optional[np.ndarray] = None,
) -> np.ndarray:
"""Convert ASTRA geometry specificiation to a SCICO projection matrix.

Convert ASTRA volume and projection geometry into a SCICO X-ray
projection matrix, assuming "parallel3d_vec" format.
"""Convert X-ray geometry specification to a SCICO projection matrix.

The approach is to locate 3 points in the volume domain,
deduce the corresponding projection locations, and, then, solve a
Expand All @@ -450,13 +458,19 @@ def convert_to_scico_geometry(
det_spacing: Spacing between detector elements. See the
`astra documentation <https://www.astra-toolbox.com/docs/geom3d.html#projection-geometries>`__
for more information.
angles: Array of projection angles in radians.
vectors: Array of geometry specification vectors.
angles: Array of projection angles in radians, mutually
exclusive with `vectors`.
vectors: Array of ASTRA geometry specification vectors, mutually
exclusive with `angles`.

Returns:
(num_angles, 2, 4) array of homogeneous projection matrices.

"""
if angles is not None and vectors is not None:
raise ValueError("`angles` and `vectors` are mutually exclusive.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps "... mutually exclusive parameters" for clarity?

if angles is None and vectors is None:
raise ValueError("One of `angles` and `vectors` must be provided.")
vol_geom, proj_geom = XRayTransform3D.create_astra_geometry(
input_shape, det_count, det_spacing=det_spacing, angles=angles, vectors=vectors
)
Expand Down Expand Up @@ -577,8 +591,10 @@ def __init__(
det_spacing: Spacing between detector elements. See the
`astra documentation <https://www.astra-toolbox.com/docs/geom3d.html#projection-geometries>`__
for more information.
angles: Array of projection angles in radians.
vectors: Array of geometry specification vectors.
angles: Array of projection angles in radians, mutually
exclusive with `vectors`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps "... radians. This parameter is mutually exclusive with ...", and likewise for the line below?

Also, before "Args:", "These options are ..." would be better as "These parameters are ..."

vectors: Array of ASTRA geometry specification vectors, mutually
exclusive with `angles`.

Raises:
RuntimeError: If a CUDA GPU is not available to the ASTRA
Expand All @@ -604,12 +620,15 @@ def __init__(

if not isinstance(det_count, (list, tuple)) or len(det_count) != 2:
raise ValueError("Expected det_count to be a tuple with 2 elements.")
if angles is not None and vectors is not None:
raise ValueError("`angles` and `vectors` are mutually exclusive.")
Copy link
Collaborator

@bwohlberg bwohlberg Feb 11, 2025

Choose a reason for hiding this comment

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

Perhaps "Parameters angles and vectors ..."? Also, exceptions typically go to a text console, so no point in adding markup. (Also applies to other exceptions in this PR.)

if angles is None and vectors is None:
raise ValueError("One of `angles` and `vectors` must be provided.")
if angles is not None:
Nview = angles.size
self.angles: Optional[np.ndarray] = np.array(angles)
self.vectors: Optional[np.ndarray] = None
else:
assert vectors is not None
if vectors is not None:
Nview = vectors.shape[0]
self.vectors = np.array(vectors)
self.angles = None
Expand Down
Loading