micropython-ulab icon indicating copy to clipboard operation
micropython-ulab copied to clipboard

Overlap and Add algorithm for fast convolution of long array

Open eleroy opened this issue 1 year ago • 7 comments

Hi everyone,

This an implementation of the oaconvolve method that may be faster than convolve espiecielly when dealing with long arrays.

Few things I'm not sure:

  • I don't think this will work without complex support and I'm not sure how to change the code to do so
  • I'm not sure there si a need for m_free at the end of the method
  • I just realized there are micropython contributing guidelines that I probably did not completely follow

The method takes the same arguments as the convolve method.

eleroy avatar Nov 25 '24 23:11 eleroy

I'm a bit confused: we agreed that oaconvolve is part of scipy.signal, so numpy.c, and filter.c shouldn't be modified at all. You should also add tests, documentation, and move the version number in ulab.c.

v923z avatar Nov 25 '24 23:11 v923z

Pff...sorry i forgot to move the method to signal, left it where i built it in filter.

I also saw it didn't built since i added the m_free that was not well used (IS it useful to free the buffer at the end or it will be free anyway when the method returns?)

I'll modify the PR to address your remarks.

eleroy avatar Nov 25 '24 23:11 eleroy

And I think that the problem of complexes has to be solved. It wouldn't make sense to include complex support for the sake of a single method.

v923z avatar Nov 25 '24 23:11 v923z

The checks fail here: https://github.com/v923z/micropython-ulab/actions/runs/12020316249/job/33509066852?pr=695#step:8:317.

v923z avatar Nov 25 '24 23:11 v923z

I also saw it didn't built since i added the m_free that was not well used (IS it useful to free the buffer at the end or it will be free anyway when the method returns?)

In this case, it shouldn't really matter.

v923z avatar Nov 25 '24 23:11 v923z

added:

  • Support without complex number
  • Documentation for oaconvolve
  • Bump ulab version
  • Test for oaconvolve (same as convolve but with reduced absolute tolerance, it seems the method is not as accurate as convolve)

eleroy avatar Nov 26 '24 19:11 eleroy

added:

* Support without complex number

* Documentation for oaconvolve

* Bump ulab version

* Test for oaconvolve (same as convolve but with reduced absolute tolerance, it seems the method is not as accurate as convolve)

Many thanks! I think it's almost ready to go. I added a couple of small comments. If those can be resolved, this could be merged.

v923z avatar Nov 26 '24 20:11 v923z