Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pygsp Laplacian calculation #8

Open
phil-hawkins opened this issue Nov 9, 2021 · 4 comments · May be fixed by #9
Open

pygsp Laplacian calculation #8

phil-hawkins opened this issue Nov 9, 2021 · 4 comments · May be fixed by #9
Labels
bug Something isn't working

Comments

@phil-hawkins
Copy link

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
Copy link
Member

mdeff commented Nov 9, 2021

Thanks for your interest and kind words!

Indeed, the class is named SphereIcosahedral. It has been renamed in epfl-lts2/pygsp@d8c4a28 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 mdeff added the bug Something isn't working label Nov 9, 2021
@phil-hawkins
Copy link
Author

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
Copy link
Member

mdeff commented Nov 10, 2021

The parameter signature of the constructor has changed though.

Right. That was done in epfl-lts2/pygsp@39a0665 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
Copy link
Author

phil-hawkins commented Dec 14, 2021

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging a pull request may close this issue.

2 participants