xbresson / spectral_graph_convnets

PyTorch implementation of spectral graph ConvNets, NeurIPS’16
MIT License
291 stars 68 forks source link

Implementation ambiguity #4

Open pinkfloyd06 opened 6 years ago

pinkfloyd06 commented 6 years ago

Hello @xbresson ,

Let me first thank for this work.

l went through your implementation. Hence , l have some question :

1- You set coarsening_levels = 4, however only L[0] and L[2] are used

x = self.graph_conv_cheby(x, self.cl1, L[0], lmax[0], self.CL1_F, self.CL1_K) and x = self.graph_conv_cheby(x, self.cl2, L[2], lmax[2], self.CL2_F, self.CL2_K) What about L[1],L[3] and L[4] ?

2- Why lmax[0] and lmax[2] are recalculated in graph_conv_cheby . Even if lmax is a parameter input of this function.

3- Do you mind explaining why your rescale Laplacian eigenvalues to [-1,1] ? rescale_L(L, lmax)

4- concat(x,x_) is not ued at all in graph_conv_cheby() :

        def concat(x, x_):
            x_ = x_.unsqueeze(0)  # 1 x V x Fin*B
            return torch.cat((x, x_), 0)  # K x V x Fin*B

5- What if K=1 in graph_conv_cheby() ? l noticed that at least K should equal 2. How can l use that just for K=1 ?

Thank you for your consideration

TheShadow29 commented 6 years ago

Hi @pinkfloyd06. I am not the author but I have read the code and I hope that I could answer some of those questions. Do note I too might have made some mistakes, feel free to correct me.

  1. The author uses graph_max_pool(x,4). In doing so the number of nodes reduces by a factor of 4 and therefore L[1] is not used.

  2. I think it doesn't matter. Either implementation would give the same result. You can compute lmax earlier and pass it as a parameter or recompute it inside the forward function. I am not sure if one of the two is better than the other in speed.

  3. I am clueless. Normalized laplacian has eigen values in the range 0 to 2. I am not sure why the range has been made -1 to 1.

  4. Yes. concat function has not been used.

  5. I am not very sure on this, but I think the line 232 should be if K >= 1 which should take care of the case.

pinkfloyd06 commented 6 years ago

@TheShadow29 Thank you for your answer :

  1. I do agree with you. But what l don't understand is the fact of computing L[1] L[3] and L[4] but never been used !! From my understanding l think for the second maxpooling layer we use L[1] rather than L[2] and if we have a third maxpolling layer it will be with L[2] , forth maxpooling L[3] and so on. Please correct me if l'm wrong
TheShadow29 commented 6 years ago

@pinkfloyd06 As I earlier mentioned, the author used graph_max_pool(x, 4). So the number of nodes are divided by a factor of 4. In the computation of the laplacian matrices, the number of nodes decrease by a factor of 2. So L[1] refers to the laplacian of the graph which would be resulted when graph_max_pool(x, 2) would be used. But since we directed divide it by a factor of 4, we don't need L[1]. Instead we need L[2] which corresponds to the laplacian formed by the graph when it is divided by a factor of 4 (twice divided by factor of 2). Hope this helps.