Skip to content

ENH: allow choice of ASTRA projector type#1485

Merged
kohr-h merged 11 commits into
odlgroup:masterfrom
kohr-h:par2d_cpu_linear
Aug 12, 2019
Merged

ENH: allow choice of ASTRA projector type#1485
kohr-h merged 11 commits into
odlgroup:masterfrom
kohr-h:par2d_cpu_linear

Conversation

@kohr-h

@kohr-h kohr-h commented Mar 21, 2019

Copy link
Copy Markdown
Member

I suggest we switch to the linear for 2D parallel beam projections. The CPU line kernel gives quite bad results in terms of interpolation artifacts (@wjp: is that to be expected? I didn't imagine it to be this severe.). For comparison a projection-backprojection cycle:

Line kernel
line

Linear kernel
linear

@kohr-h

kohr-h commented Mar 21, 2019

Copy link
Copy Markdown
Member Author

Note: the images come from the ray_trafo_parallel_2d.py example.

@adler-j

adler-j commented Mar 22, 2019

Copy link
Copy Markdown
Member

Is the linear kernel matched?

@wjp

wjp commented Mar 22, 2019

Copy link
Copy Markdown
Contributor

ASTRA's CPU projectors all have matched FP and BP, yes.

@wjp

wjp commented Mar 22, 2019

Copy link
Copy Markdown
Contributor

By the way, if you're evaluating 2D CPU projection kernels, I've just added a new 2D parallel beam CPU projector to astra called distance_driven ( from http://ieeexplore.ieee.org/abstract/document/1239600/ ), which is supposedly quite nice in terms of discretization artifacts. We're still evaluating its behaviour here.

@kohr-h

kohr-h commented Mar 28, 2019

Copy link
Copy Markdown
Member Author

How about this now?

@adler-j adler-j left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This would be a step away from getting the interpolation from the space. After some thought I've concluded that I'm in favor of this.

However this means that we should fully make the transition and actually allow users to pick the method as a parameter to the RayTransform method. Could you do this instead?

@kohr-h

kohr-h commented Apr 16, 2019

Copy link
Copy Markdown
Member Author

Yes, I've done exactly that in the #1459 PR. I can move it into this one instead.

@kohr-h kohr-h force-pushed the par2d_cpu_linear branch from fde1c47 to bff22db Compare April 28, 2019 09:50
@pep8speaks

pep8speaks commented Apr 28, 2019

Copy link
Copy Markdown

Checking updated PR...

Line 64:80: E501 line too long (82 > 79 characters)

Comment last updated at 2019-07-18 16:58:02 UTC

@kohr-h kohr-h force-pushed the par2d_cpu_linear branch from bff22db to d441031 Compare April 30, 2019 22:00
@kohr-h kohr-h changed the title ENH: use linear kernel in 2D CPU parallel projectors ENH: allow choice of ASTRA projector type Apr 30, 2019
@kohr-h

kohr-h commented May 1, 2019

Copy link
Copy Markdown
Member Author

I've pulled over the changes as mentioned, ready for review.

@adler-j adler-j left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Overall looks good, but needs some minor changes.

Also needs to add tests.

Comment thread odl/tomo/backends/astra_setup.py Outdated
Comment thread odl/tomo/backends/astra_cpu.py Outdated
Comment thread odl/tomo/backends/astra_cpu.py Outdated
Comment thread odl/tomo/backends/astra_cpu.py Outdated
Comment thread odl/tomo/backends/astra_cuda.py
Comment thread odl/tomo/backends/astra_setup.py Outdated
Comment thread odl/tomo/backends/astra_setup.py
Comment thread odl/tomo/backends/astra_setup.py Outdated
Comment thread odl/tomo/backends/skimage_radon.py Outdated
Comment thread odl/tomo/backends/skimage_radon.py
@kohr-h

kohr-h commented May 3, 2019

Copy link
Copy Markdown
Member Author

Overall looks good, but needs some minor changes.

Done that.

Also needs to add tests.

Frankly I don't really know what to test. The tests would end up just copying what's in the implementation.
If at all, such tests should be in the ray transform test code, to make sure all versions run.

@kohr-h

kohr-h commented May 7, 2019

Copy link
Copy Markdown
Member Author

Ping for re-review @adler-j

@kohr-h

kohr-h commented Jul 18, 2019

Copy link
Copy Markdown
Member Author

Ping again. How about we merge this one after resolving the conflicts? I think the job is done here.

@adler-j

adler-j commented Jul 18, 2019

Copy link
Copy Markdown
Member

Merge after conflicts are resolved. Sorry about the delay, rather busy atm.

@kohr-h

kohr-h commented Jul 18, 2019

Copy link
Copy Markdown
Member Author

Merge after conflicts are resolved. Sorry about the delay, rather busy atm.

I can imagine that. Thanks for popping in here anyway :-)

I'll fix the conflicts and you keep the mouse pointer ready to click "approve"^^.

@kohr-h kohr-h force-pushed the par2d_cpu_linear branch from b6d5a4a to 0b1411e Compare July 18, 2019 16:57
@kohr-h

kohr-h commented Jul 18, 2019

Copy link
Copy Markdown
Member Author

I'll inspect the CI for errors other than the "normal" ones, after that I'd like to merge.

@kohr-h

kohr-h commented Aug 12, 2019

Copy link
Copy Markdown
Member Author

Just the "usual" errors in CI, merging.

@kohr-h kohr-h merged commit 82a0d4c into odlgroup:master Aug 12, 2019
wjp added a commit to wjp/odl that referenced this pull request Sep 30, 2019
kohr-h pushed a commit that referenced this pull request Oct 25, 2019
* Add scaling factors for ASTRA >= 1.9.9.dev

* STY: adapt for pep8speaks

* Re-order ASTRA version handling chronologically

* Add ASTRA version comments

* BUG: Fix ASTRA DensityWeighting option

Regression from #1485

* Remove ASTRA DensityWeighting option with ASTRA >= 1.9.9.dev

ASTRA 1.9.9.dev has removed this option; it is now always enabled.
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.

4 participants