search

deepsphere / deepsphere-pytorch

DeepSphere: a graph-based spherical CNN (PyTorch)
https://arxiv.org/abs/2012.15000
MIT License
152 stars 29 forks source link

pygsp Laplacian calculation #8

Open phil-hawkins opened 2 years ago

phil-hawkins commented 2 years ago

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

mdeff commented 2 years ago

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. :)

phil-hawkins commented 2 years ago

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.

mdeff commented 2 years ago

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!

phil-hawkins commented 2 years ago

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?