Bug in Healpix sampling in pygsp

[aluo-x]

Not sure the correct place to put this bug, but currently the healpix sampling is broken.

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

[aluo-x]

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.

[mdeff]

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 https://github.com/epfl-lts2/pygsp/commit/c69398b10fee3b5d834c488052330b48ecede2c8.

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.

[aluo-x]

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

[aluo-x]

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.

[mdeff]

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?

[aluo-x]

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

[aluo-x]

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

[mdeff]

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 https://github.com/epfl-lts2/pygsp/pull/43). (n_neighbors was special-casing HEALPix that's why it got removed.)

[aluo-x]

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

[mdeff]

Done! 9ee5d09c7464be35248ad2746266bbe42c593101