Bug in Healpix sampling in pygsp about deepsphere-pytorch HOT 10 CLOSED

Also the new repo is very cool! Will look more deeply into it tomorrow.

from deepsphere-pytorch.

Done! 9ee5d09

from deepsphere-pytorch.

Additionally, the current install instructions for pygsp gives a broken version for HealPix. As the kernel widths are only defined down to 32, which breaks pooling for smaller graphs.

We should move directly to the head of the new_sphere_graph branch. I also believe the currently returned lat lon for healpy are incorrect, as the healpy documentation claims that it is returning colat & lon.

from deepsphere-pytorch.

Thanks for reporting those issues!

If we pass n_neighbors=None, the code in pygsp checks for Nside when function accepts nside.

There was indeed a bug in the PyGSP.

As the kernel widths are only defined down to 32, which breaks pooling for smaller graphs.

I've expanded the range from 1 to 2048 in epfl-lts2/pygsp@c69398b.

I also believe the currently returned lat lon for healpy are incorrect, as the healpy documentation claims that it is returning colat & lon.

Right. This has been fixed.

We should move directly to the head of the new_sphere_graph branch.

Agreed. And that will fix all the issues. 🙂 I've updated the instructions in the README.

from deepsphere-pytorch.

I believe the updated instructions still give a "broken" installation.

class SphereHealpix(NNGraph):
    def __init__(self, subdivisions=2, indexes=None, nest=False)

This repo is expecting nest=True, but fails to explicitly set the nest=True

Edit: fix typo

from deepsphere-pytorch.

Specifically this line will have to be updated to

G = SphereHealpix(subdivisions, nest=True)

Additionally, the "n_neighbors" or "k" parameter should probably be exposed, the current default is 20, while 8 may be sufficient.

from deepsphere-pytorch.

Arf you're right. I changed the default in pygsp to nest=False to follow healpy's default. While it doesn't matter for the graph convolution, it does for the pooling. (I missed it because we're developing a new pooling based on interpolation between meshes of different resolutions in https://github.com/deepsphere/deepsphere-weather, which doesn't care about ordering.)

Where do you think we should expose k? Just in this call to make it more visible?

from deepsphere-pytorch.

I have no super strong opinion about the k, I feel like if people really need that kind of tuning they will find it. But if it is exposed, then in that call would be the place. (But k is less informative compared to n_neighbors).

The nested=True is more important, since pooling assumes it to be nested, will you push a fix?

Edit: typo

from deepsphere-pytorch.

Thanks! :)

Yes the nest needs to be fixed. Let's go with the k in that call too. I'll do it or merge a PR if you want to do it.

But k is less informative compared to n_neighbors

I agree. It's clear where it's defined (a kNN graph class) but not so much in the HEALPix child class (and many other child classes). We might rename it at some point (ping epfl-lts2/pygsp#43). (n_neighbors was special-casing HEALPix that's why it got removed.)

from deepsphere-pytorch.

Would appreciate a fix from you! It's nearly midnight here in the U.S.

from deepsphere-pytorch.