C++ uniformity for "models"/operators

KrisThielemans commented 4 years ago

We have a description in the UsersGuide on expected format of AcquistionModel etc. I guess we follow that scheme. However, on the C++ there's a variety of things going on. Examples

I didn't check the rest.

Obviously, it'd be better to always use the same. One important difference is that PETAcquisitionModel uses references, while Resample uses shared_ptr. I'm not sure which is best. In general, I like to avoid pointers as much as I can (by using Type& you can decide to use std::shared_ptr, boost::shared_ptr, std::unique_ptr or whatever), but possibly it doesn't matter too much. as long as we stick to something.

@rijobro @evgueni-ovtchinnikov any opinions?

Once we decide on a common way, I'd be in favour of creating an (templated) abstract class with the interface. I realise it's probably the first template we'd use in SIRF, but it seems unavoidable here). Having a base class might make the wrapping easier as well. This could look like

template <class input_type, class output_type>
class LinearOperator
{
public:
  void setup(std::shared_ptr<const output_type> output_sptr, const std::shared_ptr<const input_type> input_sptr);
  void forward(output_type& output, const input_type& input);
  backward(input_type&, const output_type&);
};

We don't have to move everything over straightaway to the common interface of course. deriving from the class would tell people if it confirms to the interface or not.

@paskino @epapoutsellis jakobsj I guess this would correspond to the CIL::Operator. Could you point us to it? Maybe you have an opinion?

paskino commented 4 years ago

The Operator class is defined here and it requires the domain_geometry and the range_geometry, which I suppose in your example you called input_type and output_type.

Also we define direct and adjoint and not forward and backward. In SIRF I have aliased the methods so they are all present, but not in CIL.

The signature (in Python) of direct and adjoint are simpler from PETAcquisitionModel


def adjoint(self,x, out=None):
    ...
def direct(self,x, out=None):
    ...

KrisThielemans commented 4 years ago

I see CIL now does have LinearOperator which adds adjoint, see here

I personally like the range/domain terminology. Possibly our audience would be more familiar with input/output, but that could be resolved by documentation.

The signature (in Python) of direct and adjoint are simpler from PETAcquisitionModel

In C++, we cannot do the out= stuff. With out, the signature effectively does the same thing (SIRF C++ convention is to have output arguments first, as that allows you to have lots of default arguments). Obviously, we can/should add

  unique_ptr<output_type> forward(const& input_type);

(or equivalent with a shared_ptr<const input_type> of course) which could be implemented at base-class level if we have a way to create the object (in CIL I suppose range_geometry().clone() or similar)

rijobro commented 4 years ago

In favour of having a base class that all inherit from to ensure consistency. Also agree that we should have both in-place and non-in-place forward/backward methods, i.e.,:

virtual void forward(output, input) = 0;
virtual void backward(output, input) = 0;

output forward(input)
{
    output = create_output();
    forward(output, input);
    return output;
}

output backward(input)
{
    output = create_output();
    backward(output, input);
    return output;
}

I also don't think we should be implementing direct or adjoint except on the python level.

Lastly don't mind if we pass arguments by reference or shared_ptr etc.

SyneRBI / SIRF

C++ uniformity for "models"/operators #737