search

TomographicImaging / CIL

A versatile python framework for tomographic imaging
https://tomographicimaging.github.io/CIL/
Apache License 2.0
95 stars 41 forks source link

CIL algebra is not compatible with numpy and optional arguments #988

Open epapoutsellis opened 3 years ago

epapoutsellis commented 3 years ago

We have some algebra reductions for DataContainers coming from numpy. However we have no tests.

In addition .sum does not return a DataContainer or ImageData when axis is not 0. This is quite important when dealing with multichannel data.

    from cil.framework import ImageGeometry

    ig = ImageGeometry(3,3,4)

    x = ig.allocate('random_int', seed=10)

    res1 = x.sum(axis=0)
    res2 = x.sum(axis=1)
    res3 = x.sum()

    print(res1)
    print(res2)
    print(res3)
[[ 94 225 235]
 [125 184 313]
 [198 148 162]]
[[ 66 112 230]
 [104 113 123]
 [192 145 151]
 [ 55 187 206]]
1684
paskino commented 3 years ago

Initially I had thought sum would reduce a DataContainer to a number. This is what happens in the default configuration. However if you pass the optional parameter axis returning the sum for numpy returns a numpy array and then we are lost. In such case, we should first deduce the size (geometry) and then return an appropriate DataContainer.

https://github.com/TomographicImaging/CIL/blob/e7398e1bbc2c765b45999ff55a9ba800dedb9b88/Wrappers/Python/cil/framework/framework.py#L2399-L2400

paskino commented 3 years ago

I found one unit test!!!!

https://github.com/TomographicImaging/CIL/blob/e7398e1bbc2c765b45999ff55a9ba800dedb9b88/Wrappers/Python/test/test_DataContainer.py#L475

epapoutsellis commented 3 years ago

There are more universal functions than sum that are not implemented correctly in CIL, e.g., max, min with axis. All these return a numpy array.

paskino commented 3 years ago

When I developed it I didn't think we'd use the axis parameter and these methods would return a number. However, we allow passing the axis parameter and also make use of it, which is wrong in the current setup.

Can you make an example of usage?

paskino commented 3 years ago

REductions are not the algebra methods like add etc. Are those also not taking the optional (hidden) numpy parameters and return something wrong?

paskino commented 2 years ago

@epapoutsellis I guess we need specific examples of usage for each function we want to "fix".

I currently think we shouldn't use all of the numpy parameters if we don't have to. In fact, if we remove *args, **kwargs from sum it will do what I developed it to do and it won't have the current bug.

Additionally, passing axis as a number is against our normal philosophy of passing the label rather than the index of the axis.