SyneRBI / Hackathon-SIRF

SIRF fork for hackathon
Other
1 stars 0 forks source link

Unify data containers: CIL <=> SIRF #5

Open mehrhardt opened 5 years ago

mehrhardt commented 5 years ago

I encountered a new issue with the CIL-SIRF compatibility. The following example

import os
import shutil
import pSTIR as pet

os.chdir(pet.petmr_data_path('pet'))
shutil.rmtree('working_folder/thorax_single_slice',True)
shutil.copytree('thorax_single_slice','working_folder/thorax_single_slice')
os.chdir('working_folder/thorax_single_slice')

image = pet.ImageData('emission.hv');
image_array=image.as_array()*.05
image.fill(image_array);

from ccpi.optimisation.funcs import IndicatorBox
g = IndicatorBox(lower=0, upper=1)               

image_new = g.prox(image, 1)

results in

Traceback (most recent call last):

  File "<ipython-input-2-2381c0694166>", line 4, in <module>
    image_new = g.prox(image, 1)

  File "/home/sirfuser/devel/install/python/ccpi/optimisation/funcs.py", line 247, in prox
    return  (x.maximum(self.lower)).minimum(self.upper)

AttributeError: 'ImageData' object has no attribute 'maximum'
paskino commented 5 years ago

Is the method maximum relevant? Should it be a method of DataContainer or could we create a function that does this job?

KrisThielemans commented 5 years ago

We did discuss if we should add various functions like this (max/min/norm etc) and voted for not doing it to keep SIRF lightweight. Obviously, we' re adding more stuff all the time (including arithmetical operators) so I suppose it makes sense now.

If we do go ahead, I think the DataContainer should have virtual methods such that it can call the relevant function from the underlying container (e.g. STIR has find_max for images). This would be faster then first copying the stuff somehwere and then determining the max. At first sight, we could have a default implementation that calls as_array, but interestingly as_array is Python/MATLAB specific (while the C++ code obviously isn't), so I wouldn't do that. It could be done once we have iterators for DataContainers, see https://github.com/CCPPETMR/SIRF/issues/212.

Final thing to consider then is naming etc. You'd want to be close to the native feel. Sadly, in MATLAB max just works on 1 dimension of the array, so I suppose there is no native equivalent for maximum. numpy has amax which defaults to axis=None to use everything, but users might expect a SIRF function to be able to specify axis as well, which would really lead us to far.

Oh, and of course, it'd have to throw if the data is complex.

so, not as trivial as you'd hope.

mehrhardt commented 5 years ago

I suggest to close this issue as we have a new one in SIRF.