search

MichaelChirico / r-bugs

A ⚠️read-only⚠️mirror of https://bugs.r-project.org/
20 stars 0 forks source link

[BUGZILLA #408] convolution bug #566

Open MichaelChirico opened 4 years ago

MichaelChirico commented 4 years ago

From: wsimpson@<::CENSORED -- SEE ORIGINAL ON BUGZILLA::> Full_Name: Bill Simpson Version: 65.1 , 0.90.1 OS: Linux Submission from: (NULL) (193.62.250.209)

I reported this on r-help, but here is official bug report.

The present convolve() does not do convolution by default. Its default behaviour is correlation. This is a bug.

The default argument conj should be set to FALSE.

The zero-padding should be on the right for linear convolution (don't ask me why you call this type="open"; I suggest type="linear").

Here is what I expect linear convolution to do: myconvolve<-function (x,h) { nx <- length(x) nh <- length(h)

zero pad

if(nx>nh)
   {
   x <- c(x,rep(0, nh-1))
   h<-c(h,rep(0,nx-1))
   }
else
   {
   h <- c(h,rep(0, nx-1))
   x<-c(x,rep(0,nh-1))
   }
x <- fft(fft(x) * fft(h), inv = TRUE)
Re(x)/length(x)

I am not sure about this, the R IFFT is weird

}

What "circular" convolution should do is just eliminate the zero-padding:

myconvolve2<-function (x,h) {

no padding, circular convolution

nx <- length(x)
nh <- length(h)
x <- fft(fft(x) * fft(h), inv = TRUE)
Re(x)/(nx)

}

I suggest that you create two functions, convolve() and correlate(), and get rid of the conj argument in convolve().

METADATA

MichaelChirico commented 4 years ago

NOTES: Is this a bug?

METADATA

MichaelChirico commented 4 years ago

Audit (from Jitterbug): Wed Feb 16 19:28:42 2000 ripley moved from incoming to Language Thu Aug 12 01:36:19 2004 thomas changed notes

METADATA

MichaelChirico commented 4 years ago

I think this can be closed; myconvolve seems equivalent to:

function(x, h) convolve(x, rev(h), type = "open")

and myconvolve2 is equivalent to:

function(x, h) convolve(x, h, conj = FALSE, type = "circular")

METADATA

MichaelChirico commented 4 years ago

Comment 3 and the manual entry for the 'convolve' function are not entirely correct. Specifically, the following two

convolve(x, y, conj=FALSE, type="o")
convolve(x, rev(y), type="o")

are equivalent only for real-valued sequences except for the shifts, and they are not equivalent for complex-valued ones.

Finding an equivalence for the latter case still leaves the problem pointed out by the reporter unresolved. In my view, the reporter is stating that 'convolve' is by default computing a correlation, and thus finds its name decieving and error prone. Instead, the manual entry seems to treat both correlation and convolution as just variations of a more general concept. The latter may be the case in some fields, but even then, I feel the reporter has a point: setting conj=TRUE does not compute the usual convolution as the manual entry suggests and it is deceiving that 'convolve' by default computes correlations.

In passing by, the explanation of the conj argument is not entirely clear without indicating what is being conjugated. It seems to indicate that Conj(fft(x)*fft(y)) is being taken instead of Cong(fft(y)).

METADATA

MichaelChirico commented 4 years ago

Hugo you are not incorrect; however, the help page also says:

x, y: numeric sequences _of the same length_ to be convolved.

so the documentation is silent on the extent to which complex-valued sequences are even supported. As a reminder:

x <- runif(10) + runif(10) * 1i
is.numeric(x)

[1] FALSE

METADATA

MichaelChirico commented 4 years ago

On the other hand, looking at the source, it does seem clear that the authors explicitly intended full support for non-real x and/or y since way back in 1999:

convolve <- function(x, y, conj=TRUE, type=c("circular","open","filter")) { type <- match.arg(type) n <- length(x) ny <- length(y) Real <- is.numeric(x) && is.numeric(y)

...so it seems the documentation hasn't yet caught up with the code in this respect.

METADATA

github-actions[bot] commented 4 years ago

So is this a case where documentation change is needed since after 20 years the function name is not likely to be changed , but the documentation issue about complex numbers (not directly part of the original report but discovered in analysis of the issue) should be fixed?

METADATA