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

Changes to linop.xray.astra module #551

Merged
merged 7 commits into from
Sep 10, 2024
Merged

Changes to linop.xray.astra module #551

merged 7 commits into from
Sep 10, 2024

Conversation

bwohlberg
Copy link
Collaborator

Changes to linop.xray.astra module:

  • Add missing typing
  • Fix some docstrings
  • Change interface of convert_to_scico_geometry

Note:

  • Typing still needs some attention. In particular, it's not always clear whether jax or numpy arrays should be specified, and jax.ArrayLike should perhaps be replaced by numpy.typing.ArrayLike.
  • Some functions do not have any docstrings.

@Michael-T-McCann
Copy link
Contributor

* Typing still needs some attention. In particular, it's not always clear whether `jax` or `numpy` arrays should be specified, and `jax.ArrayLike` should perhaps be replaced by `numpy.typing.ArrayLike`.

Putting the specific issue of this module aside, do we have a policy about this for SCICO? For example, jax.numpy functions are annotated with jax._src.typing.ArrayLike -> jax._src.typing.Array (documented here, includes NumPy inputs), whereas LinearOperator.__call__ is Union[LinearOperator, Array, BlockArray] -> Union[LinearOperator, Array, BlockArray] (does not include NumPy inputs).

For this module, it would be convenient if at least JAX inputs were allowed for everything, with internal conversion to NumPy when ASTRA requires it.

@bwohlberg
Copy link
Collaborator Author

Putting the specific issue of this module aside, do we have a policy about this for SCICO? For example, jax.numpy functions are annotated with jax._src.typing.ArrayLike -> jax._src.typing.Array (documented here, includes NumPy inputs), whereas LinearOperator.__call__ is Union[LinearOperator, Array, BlockArray] -> Union[LinearOperator, Array, BlockArray] (does not include NumPy inputs).

I don't recall this has ever been discussed in detail. If both jax and numpy array inputs are reasonable, then a Union would make sense. It's often clear, though, what the output type will be if there's an explicit jax.numpy or numpy call in the processing chain.

For this module, it would be convenient if at least JAX inputs were allowed for everything, with internal conversion to NumPy when ASTRA requires it.

That seems fine.

@bwohlberg bwohlberg merged commit 6c01c4d into mike/3d_xray Sep 10, 2024
4 checks passed
@bwohlberg bwohlberg deleted the brendt/3d_xray branch September 10, 2024 23:39
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