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

Add JAX 3D X-ray CT projector #529

Merged
merged 40 commits into from
Sep 16, 2024
Merged

Add JAX 3D X-ray CT projector #529

merged 40 commits into from
Sep 16, 2024

Conversation

Michael-T-McCann
Copy link
Contributor

@Michael-T-McCann Michael-T-McCann commented Jun 7, 2024

See example results at https://scico--529.org.readthedocs.build/en/529/examples/ct_projector_comparison_3d.html
Still not clear why the back projections don't look like they match. It seems like calling .transpose(2, 1, 0) on the ASTRA back projection gets them closer. My best guess is that this example generates vectors that cause ASTRA back projection to get flipped. I do think SCICO's back projection is correct given that it comes from autograd.

For consideration:

  • Is the XRayTransform(Parallel3dProjector(in_shape, P, out_shape)) syntax too complex? I like that it separates the projector logic from SCICO (makes it easy to cut and paste into other projects), but it's awkward.
    • Removed XRayTransform

@bwohlberg bwohlberg added the enhancement New feature or request label Jun 7, 2024
docs/source/examples.rst Outdated Show resolved Hide resolved
scico/linop/xray/_xray.py Outdated Show resolved Hide resolved
scico/linop/xray/_xray.py Outdated Show resolved Hide resolved
scico/linop/xray/_xray.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@bwohlberg bwohlberg left a comment

Choose a reason for hiding this comment

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

Very nice. I added a few nitpicking comments/suggestions.

Copy link

codecov bot commented Jun 7, 2024

Codecov Report

Attention: Patch coverage is 27.02703% with 54 lines in your changes missing coverage. Please review.

Project coverage is 93.51%. Comparing base (d9026d3) to head (d182530).
Report is 18 commits behind head on main.

Current head d182530 differs from pull request most recent head 308b602

Please upload reports for the commit 308b602 to get more accurate results.

Files Patch % Lines
scico/linop/xray/_xray.py 30.23% 30 Missing ⚠️
scico/linop/xray/astra.py 16.67% 20 Missing ⚠️
scico/examples.py 20.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #529      +/-   ##
==========================================
- Coverage   94.67%   93.51%   -1.16%     
==========================================
  Files          91       90       -1     
  Lines        5707     5780      +73     
==========================================
+ Hits         5403     5405       +2     
- Misses        304      375      +71     
Flag Coverage Δ
unittests 93.51% <27.03%> (-1.16%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bwohlberg
Copy link
Collaborator

bwohlberg commented Jun 7, 2024

  • Is the XRayTransform(Parallel3dProjector(in_shape, P, out_shape)) syntax too complex? I like that it separates the projector logic from SCICO (makes it easy to cut and paste into other projects), but it's awkward.

This is worth discussing. It may be worth hiding the Parallel3dProjector construction within the XRayTransform initializer. Maintaining the separation of the projector logic does seem worth maintaining, at least for now.

  • Currently we have functions that covert from the SCICO projection specification to ASTRA one. Should we add functions that convert from a SCICO projection object to an ASTRA one? Where should these live?

Also worth discussing. The natural home for this would seem to be scico.linop.xray.astra.

  • I find the relative positions of scico.linop.xray.astra.XRayTransform3D and scico.linop.XRayTransform in the module hierarchy a little odd. Time for a reorg (maybe after merging this)?

Is the concern that astra and svmbir are submodules of scico.linop.xray.astra? If so, I think this is justified by the view that the ones in xray are "standard", and the ones in xray.astra and xray.svmbir are additional options provided by other packages. But we can discuss this too. But I agree that we should revisit whether the "standard" transform should be imported from scico.linop.xray rather than scico.linop as is currently the case - it does seem a bit strange for the "standard" transform to have a home two levels up the module tree from the imported ones.

scico/linop/xray/_xray.py Outdated Show resolved Hide resolved
scico/linop/__init__.py Outdated Show resolved Hide resolved
bwohlberg and others added 4 commits September 10, 2024 17:39
* Add typing, fix docstrings, change  convert_to_scico_geometry interface

* Improve docs

* Bug fix

* Fix function call in test
@Michael-T-McCann Michael-T-McCann merged commit 5d5b2b9 into main Sep 16, 2024
18 checks passed
@Michael-T-McCann Michael-T-McCann deleted the mike/3d_xray branch September 16, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants