search

dmpelt / pyastratoolbox

Python interface to the ASTRA-toolbox
4 stars 3 forks source link

create_sino without projector #7

Closed valeriysokolov closed 10 years ago

valeriysokolov commented 10 years ago

I wanted to make a sinogram for fanflat_vec geometry and it appeared that it is not well-supported by projectors. But also it is possible to get sinogram by FP_CUDA, having only projection and volume geometries without projector itself.

I made a fork and put the modification enabling that behaviour here: https://github.com/valeriysokolov/pyastratoolbox/commit/6f422ce3e445fc34b890dddaf446418eb43ab5eb

I am new to pyastratoolbox and to astra-toolbox, maybe I did something overcomplicated. Could you please take a look at the modification. I will be happy to know about better solutions.

If this modification is useful, maybe I should make a pull request or something like that.

dmpelt commented 10 years ago

Looks like a good improvement to me! Thanks for writing it. If you make a pull request, I can merge it into the master branch.

I tried out the code, and there is just one small bug: Line 382 should say raise NotImplementedError instead of raise NotImplemented. However, I think that something like raise ValueError("""A ``proj_id`` is needed when CUDA is not used.""") is more appropriate, because the error is not that something is not implemented, but that not enough parameters are given.

Again, thanks for the help! If you have any more improvements, feel free to create pull requests for them too.

valeriysokolov commented 10 years ago

Thank you for pointing at the bug with an exception. I used your suggestion and added this change. Also I made a pull request: https://github.com/dmpelt/pyastratoolbox/pull/8

As about more improvements, for now I see only technical places to be improved, like PEP-8 conformance or maybe use of **kwargs in some functions (which will break exisisting interfaces), and I am in doubt about necessity of such modifications. But, if any real improvements will be made, surely I will create pull requests for them.

dmpelt commented 10 years ago

Merged!

Thanks for the help!