search

epfl-lts2 / pygsp

Graph Signal Processing in Python
https://pygsp.rtfd.io
BSD 3-Clause "New" or "Revised" License
488 stars 93 forks source link

Issue in compute_jackson_cheby_coeff #74

Open jiwoongpark92 opened 4 years ago

jiwoongpark92 commented 4 years ago

It seems to have to be corrected from ch[0] = (2/(np.pi))*(np.arccos(filter_bounds[0])-np.arccos(filter_bounds[1])) to ch[0] = (1/(np.pi))*(np.arccos(filter_bounds[0])-np.arccos(filter_bounds[1])).

nperraud commented 4 years ago

Thanks for reporting this.

@mdeff do you know why this definition was used? I remember we had a big discussion about the definition when working on a student project together as it was not matching the definition of chebynet.

mdeff commented 3 years ago

Yes the first coefficient is doubled in the PyGSP. In the ML code I wrote I did not double it indeed. The code in the PyGSP is a straight translation from matlab, which also doubles the first coefficient. Do you see any reason to double it @nperraud? Did you write the matlab code?

I was going to not double the first coefficient in my overhaul of the Chebyshev approximation in the polynomial-approximations branch.

This choice doesn't impact the user as long as it's consistent (between the code that computes the coefficients and the one that applies them). We should check what other implementations do.

nperraud commented 3 years ago

The Chebyshef implementation of the gspbox was taken from the code of the spectral wavelet paper from 2012. I do not know why this two is there and it might even be a mistake. I believe, it was intentionally done so, but I have no clue why.

mdeff commented 3 years ago

Thanks for the historical pointer. :) I don't think there's a mistake. It's a matter of definition. A factor of two must appear for the first coefficient. The coefficient is divided by 2 either when used (current PyGSP behavior) either when computed (the ML code, because the coefficient is learned not computed). I have a slight preference for the second. But is there a generally accepted definition?

nperraud commented 3 years ago

For reference here is the spectral graph wavelet toolbox: https://wiki.epfl.ch/sgwt And here is the spectral graph wavelet paper: https://www.sciencedirect.com/science/article/pii/S1063520310000552 The following shows the definition used for the Chebychev coefficient. image The pi/2 in eq. 52 and the 1/2 in eq. 53 should explain the convention used in the code.

Now I have not gone deeply to know why they chose this definition. I think we "simply" need to reference properly the definition we choose to use.

mdeff commented 3 years ago

The pi/2 in eq. 52 and the 1/2 in eq. 53 should explain the convention used in the code.

Agreed.

Now I have not gone deeply to know why they chose this definition. I think we "simply" need to reference properly the definition we choose to use.

Agreed. As numpy went without the factor 2 in the evaluation (i.e., like the ML code, unlike the current pygsp), I think we should follow this convention for consistency (and least surprise to users). Maybe even use numpy to compute the coefficients and evaluate the polynomial.

nperraud commented 3 years ago

Agreed, let us be compatible with numpy