weave.inline to cython with external library

pik-copan / pyunicorn

Unified Complex Network and Recurrence Analysis Toolbox

http://pik-potsdam.de/~donges/pyunicorn/

Other

195 stars 86 forks source link

weave.inline to cython with external library #89

Closed harmening closed 6 years ago

harmening commented 7 years ago

Example for test_pearson_correlation() function from timeseries.surrates.py

jdonges commented 7 years ago

Looks good to me. Boyan, if you agree I would merge this pull request.

ntfrgl commented 7 years ago

Is there any reason to keep our own implementation of PCC if it is available within numpy? How does the performance compare?
In general, I don't think there is any use in keeping "slow" and "fast" versions around, once the decision is made.
Why the name src_fast_surrogate.c? I would rather choose something specific to PCC, or, in case we really want backend implementations of different methods in the same C file, something general, akin to the numerics.pyx.

harmening commented 7 years ago

Our own implementation of PCC (the fast one) is faster for smaller data, but numpy's corrcoef function is faster when the input data is larger than approx. 110timeserie a 110 values. So we could at least keep the "fast" version.
Could be that the "slow" version was needed for numpy-arrays that are not of C type and not contiguous? (See old pyunicorn/timeseries/surrogates.py l.659). By calling the function with array.copy(order='c') the arrays should be C-contiguous now
Originally I wanted to create one c-file for each class in timeseries. But I can also put all c source code into sth like src_numerics.c .

ntfrgl commented 7 years ago

I agree on the order='c' point. Maybe the method should dispatch to either our fast implementation or the numpy one, depending on input size.
The numerics.c are huge, generated files. They are not supposed to be in the repository.

ntfrgl commented 7 years ago

Please use r() to round explicit arrays in doctests, which makes them more robust. Example: core.Network.degree_distribution(). Defined in conftest.py.

ntfrgl commented 6 years ago

LGTM!

To finish up this PR, please do the following:

Remove the unnecessary _slow versions.
Merge with latest master, which contains CI tests for the latest versions of numpy, scipy and Cython.
Add a short description to docs/source/changelog.rst under the next version 0.6.
Add yourself to CONTRIBUTIONS.rst.

Thank you!