search

skuschel / postpic

The open-source particle-in-cell post-processor.
GNU General Public License v3.0
66 stars 27 forks source link

Bug in version update, wrong number of arguments #261

Closed skuschel closed 3 years ago

skuschel commented 3 years ago 
Python v3.9.2
scipy version 1.6.2

Hi @Ablinne I noticed, that the tests stopped working with a version update. The reason is a change of argument number of scipy.ndimage._nd_image.geometric_transform, which happens for example here, but also in other places. https://github.com/skuschel/postpic/blob/master/postpic/helper.py#L287

The all have the same error. The important part reads:

  File "/home/stephan/Work/Software_Physik/postpic.git/postpic/helper.py", line 287, in map_coordinates_chunk
    _nd_image.geometric_transform(sub_filtered, None, sub_coordinates, None, None,
TypeError: function takes exactly 12 arguments (11 given)

I tried to fix it myself, but I was unable to find the documentation of the C function you are using. Do you think that can be changed to a scipy function with a defined interface, like scipy.ndimage.map_coordinates ?

Cheers Stephan

Here is the full log: test.log

Ablinne commented 3 years ago

That low-level function is the actual backend of scipy.ndimage.map_coordinates. The original version just called that, but got really slow for big fields. So what I did was look up the implementation of scipy.ndimage.map_coordinates and basically copied it, only adding the ability to split the work into chunks to be run in parallel. So maybe we need to go back to that implementation and see what they changed. Or try and see if their implementation is also faster now...

skuschel commented 3 years ago

I see. So what happened is that they added some padding when splines are used (which we are using too): https://github.com/scipy/scipy/blob/v1.6.1/scipy/ndimage/interpolation.py#L356-L458

So I guess the speedup was quite significant, as the the normal map_coordinates always allocates new space for the output array, which is not a great choice, when the array is chunked for parallel processing.

The responsible change in scipy is this: https://github.com/scipy/scipy/commit/9c299da04b9c419786a56c8e657aaabeb35f9069

I think its save to assume, there was no padding before, so I will just add a zero and check the results. Thanks @Ablinne