open2c / cooltools

The tools for your .cool's
MIT License
135 stars 51 forks source link

Rarely reproducible error with fftconvolve for expected_cis #362

Open agalitsyna opened 2 years ago

agalitsyna commented 2 years ago

clever solution that immensely speeds up the expected_cis calculation is to do fast Fourier convolution for calculation of pairwise distances between bad bins (here). However, at least with latest scipy version 1.8.1, there is a rarely encountered problem that fftconvolve produces NaNs for all of the output (it is not stochastic, but happens for some shapes of bad bins vectors). Because we explicitly convert nans to integers, they become negative very large numbers. (noticed by @kannandeepti ) I do not know what might be a problem with fftconvolve, but maybe we can pass an option to calculate regular convolution?

E.g. this scipy.signal.convolve(x, x[::-1], mode="full", method="direct") will produce no error if used instead of: https://github.com/open2c/cooltools/blob/ab5d775ee50fb3d4483520a40f758914348e89b7/cooltools/api/expected.py#L57

nvictus commented 2 years ago

As of v0.19, convolve automatically chooses this method or the direct method based on an estimation of which is faster.

https://docs.scipy.org/doc/scipy/reference/generated/scipy.signal.fftconvolve.html

Maybe replace anyway?