Closed Michael-T-McCann closed 2 weeks ago
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.:exclamation: 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 :warning: |
scico/linop/xray/astra.py | 16.67% | 20 Missing :warning: |
scico/examples.py | 20.00% | 4 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
- 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
andscico.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.
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:
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.XRayTransform