deepsphere-pytorch icon indicating copy to clipboard operation
deepsphere-pytorch copied to clipboard

pygsp Laplacian calculation

Open phil-hawkins opened this issue 2 years ago • 4 comments

Thank you for for the great research on spherical graphs.

In https://github.com/deepsphere/deepsphere-pytorch/blob/master/deepsphere/utils/laplacian_funcs.py the code attempts to import the class SphereIcosahedron and later call the method compute_laplacian on it.

I have installed the version of pygsp as per the readme instructions:

pip install git+https://github.com/epfl-lts2/pygsp.git@39a0665f637191152605911cf209fc16a36e5ae9#egg=PyGSP

However this version has no class SphereIcosahedron

phil-hawkins avatar Nov 09 '21 07:11 phil-hawkins

Thanks for your interest and kind words!

Indeed, the class is named SphereIcosahedral. It has been renamed in https://github.com/epfl-lts2/pygsp/commit/d8c4a28ca279609163f64fb3492c05da19e85e62 to have a more uniform naming. Could you try to rename SphereIcosahedron to SphereIcosahedral? If that works, I'd be happy to merge a PR. :)

mdeff avatar Nov 09 '21 15:11 mdeff

Could you try to rename SphereIcosahedron to SphereIcosahedral? If that works, I'd be happy to merge a PR. :)

OK, got it. The parameter signature of the constructor has changed though. I'm assuming the new subdivisions argument can be calculated as 2n where n is the order of the Icosahedron?

There are a few other minor updates required that I'll add to a PR.

phil-hawkins avatar Nov 10 '21 02:11 phil-hawkins

The parameter signature of the constructor has changed though.

Right. That was done in https://github.com/epfl-lts2/pygsp/commit/39a0665f637191152605911cf209fc16a36e5ae9 to unify the parameters across the multiple discretizations of the sphere.

I'm assuming the new subdivisions argument can be calculated as 2ⁿ where n is the order of the Icosahedron?

Correct. While subdivisions must be a power of 2 in the current implementation, the API change allows for a more general implementation in the future if needed.

We did actually update the required PyGSP version to fix a bunch of issues (#7). But those API changes got overlooked.

Looking forward to the PR. Many thanks @phil-hawkins!

mdeff avatar Nov 10 '21 04:11 mdeff

The project runs now, however it seems like the performance isn't where it should be, at least with the default config with respect to the results in the paper. I'm not sure if this related to the config, my changes or something earlier.

This is the result form the final epoch run with default config: Starting Epoch: 30 beginning validation epoch Average precisions: [0.99981521 0.29534943 0.87257515] mAP: 0.5839622866476017

Do you have any ideas on this @mdeff?

phil-hawkins avatar Dec 14 '21 00:12 phil-hawkins