SyneRBI / SIRF

Main repository for the CCP SynerBI software
http://www.ccpsynerbi.ac.uk
Other
59 stars 29 forks source link

Resampler norm #1186

Closed evgueni-ovtchinnikov closed 1 year ago

evgueni-ovtchinnikov commented 1 year ago

Changes in this pull request

Implements norm method for NiftyResampler.

Testing performed

Related issues

Fixes #1182

Checklist before requesting a review

Contribution Notes

Please read and adhere to the contribution guidelines.

Please tick the following:

KrisThielemans commented 1 year ago

I realise of course this is WIP, but some comments.

Why do we need the Wrapped_sptr? I can see it's convenient to have norm() etc on a pointer, but why not simply dereference the shared_ptr? I guess I'm missing something.

Also BFOperator could be useful I guess, but in its current form, it could just as well be a lambda function probably.

I'm a bit doubtful of including extra classes just for this purpose. If we do, they'd have to be generalised and put in other places once it works.

evgueni-ovtchinnikov commented 1 year ago

I just tried to keep modifications to JacobiCG and AcquisitionModel classes minimal. In my implementation of JacobiCG, vector_type objects are assumed to have certain methods (dot and axpby, now also norm), and shared pointers do not have them. An alternative to my approach would be to assume all vector_type objects to be pointers, but I found it to be too restrictive an assumption about the nature of vector_type, and it would require changing all current definitions of BFOperator and norm for acquisition models, whereas introducing a simple wrapper object for shared pointers allowed me to avoid it.