much improved NN graph by mdeff · Pull Request #43 · epfl-lts2/pygsp

mdeff · 2019-02-25T00:45:53Z

Contains PR #21 with some improvements.

Major fixes:

correct symmetrization of distance matrix (695272b)
squared width in gaussian kernel (f544e1e)
FLANN returns distance^p for a p-norm (8cc3539)

Major improvements:

much better and complete documentation
better and more complete tests
feature standardization (bf7427f)
radius estimation (26e12e3)
user choice of similarity kernel (9c8e86e)

Ohers:

width = radius / 2 for radius graphs (011dc81)

- do not use underscores in functions args

…nce -> fix fix radius nn graph for spdist

try to install via before_install

# Conflicts: # pygsp/graphs/nngraphs/nngraph.py # pygsp/tests/test_graphs.py

…efactor # Conflicts: # README.rst

Nmslib

nperraud · 2019-07-22T09:09:01Z

I have just added the function I coded to this branch to get the nearest neighbor search out of the Knn graph. The Knn graph function still needs to be modified.

mdeff · 2019-07-23T09:06:02Z

Thanks! Can you fix the tests?

nperraud · 2019-07-23T09:14:13Z

I tried, but failed. They work on my machine but the seed is different. I have to think about how to do it... Maybe you can help me. Let us discuss later...

nperraud · 2019-07-26T12:33:23Z

Tests are passing... The coverage is reduced (probably less raise Error check...) but I think it will increase again once the functions in _nearest_neighboors are cleaned. I wil do that before my vacation...

Currently the code is not very clean...

nperraud · 2019-08-17T11:24:07Z

Ok Should be ready for the merge...

nperraud · 2019-08-19T09:02:09Z

@mdeff Everything is done. For me, it can be merged to master.

gcuendet · 2019-10-17T10:08:16Z

Very nice to see more efficient knn search implementations for knn graph building!
Is there anything still missing before merging the PR? Do you have a rough idea of when this will be merged?
Thanks for all the hard work on this very useful python package!

mdeff · 2019-10-17T11:48:14Z

Thanks for the kind words @gcuendet. I mostly need to address review comments. I would also like to merge knn and radius graphs. I'll probably get to this in the not too far future as two other PRs (#55 and #57) depend on it. Then I'll make a new release.

gcuendet · 2019-10-17T14:13:02Z

pygsp/_nearest_neighbor.py

+    if kind == 'knn':
+        params['k'] = k + 1
+    elif kind == 'radius':
+        params['k'] = features.shape[0]  # number of vertices


I think there is a problem here. This does not scale.
According to the doc of scipy.spatial.cKDTree.query

If x has shape tuple+(self.m,), then d has shape tuple+(k,).

Which means tree.query() is going to return a huge matrix of NxN.

This is actually different from scipy.spatial.KDTree.query which, according to the doc, returns :

If k is None, then d is an object array of shape tuple, containing lists of distances.

Which, if I understand correctly, means that tree.query() is going to return an array of size N containing lists with varying length, depending on the number of neighbours in the epsilon ball.

Anyway, I am testing this locally with the fountain dataset that you made available here and it seems that constructing a radius NN graph using this branch of pygsp crashes

G = graphs.NNGraph(points, kind='radius', radius=0.01, standardize=False, kernel_width=0.1)

whereas using the pip version of pygsp does not.

G = graphs.NNGraph(points, NNtype='radius', epsilon=0.01, rescale=False, sigma=0.1)

On my initial experiments, I also used scipy.spatial.cKDTree.query_ball_point() which only returns the indices, so distances need to be computed seperately. That might be an alternative maybe?

See PR #60 about the use of scipy.spatial.cKDTree.query_ball_point()

mdeff · 2020-12-18T19:01:52Z

rebase or merge? Too long history for rebase, and would make things even more difficult for sphere-graphs and learned-graph.
NNGraph is a bad name: Graph is redundant, and NN isn't explicit. pg.graphs.NearestNeighbors? (Also suggested in Add a graph learning module #57 (comment).) Other ideas?
kernel_width => scale? See much improved NN graph #43 (comment)
squash splitting of nn functions (for learnedgraph) and move the file to pygsp.graphs
update twomoons (kwargs to NNGraph, variance?), see much improved NN graph #43 (comment) and much improved NN graph #43 (comment)
good automatic scale for radius graph, see much improved NN graph #43 (review) and much improved NN graph #43 (comment)
merge metric and order parameters => it's minkowski if int, names are aliases
remove kind, see much improved NN graph #43 (comment)
address the issue raised by @gcuendet

nperraud · 2020-12-19T14:30:03Z

I suggest the following:

NearestNeighbors instead of NNGraph
I find all possibilities bad for the variable kernel_width because it is linked to the gaussian function. So I am not against scale, but I am not pushing for it...
Do yo want to do learn graph is the same PR?

mdeff · 2020-12-19T15:07:21Z

Let's go with NearestNeighbors then. :)
Agreed. Some synonyms for ideas: spread, extent, size, width, span, stretch. Maybe we should think of it as a step before the kernel, as in the end we do similarity = kernel(distance / scale). So it's acting on the distances. What about something that "adjusts" the metric? Like expansion, contraction, inflation, amplification, growth, dilation. Contraction of space/distances = expansion of the kernel.
learned-graph stays in its own PR, as does sphere-graphs.

nperraud · 2020-12-19T17:08:13Z

Seeing all your proposition, I believe scale is the best option. It is rescaling or renormalisation, but I would prefer not to use something like norm because it is confusing.

naspert and others added 30 commits March 19, 2018 10:57

attempt to refactor nn graph building

1ee85b5

update tests

4bacd5c

fix typo

00bbcdd

fix tests (avoiding not implemented combinations)

b822333

- fix missing space after colon in dictionary

38aebd0

- do not use underscores in functions args

fix (matlab) GSP url

524c60f

throw exception when using FLANN + max_dist (produces incorrect results)

ae83814

update test case to fit FLANN & max_dist exception

62fc0ce

implement nn graph using pdist using radius

6f473fa

implement radius nn graph with flann

25ec6d2

flann returns the squared distance when called with 'euclidean' dista…

96b628e

…nce -> fix fix radius nn graph for spdist

compute sqrt of list properly

09bbff4

use cyflann instead of pyflann (radius search not working)

27b9a03

check nn graphs building against pdist reference

8a1f9b9

cyflann needs the flann library to be installed on the system

6e9e2ac

try to install via before_install

check nn graphs building against pdist reference

811de06

backport stuff from cyflann branch

813fe39

flann should (mostly) work for knn graphs

4a4d597

fix pdist warnings

53dffc1

implement and use scipy-ckdtree as default (faster than kdtree)

1309e92

Merge remote-tracking branch 'origin-nas/nn_cyflann' into nn_refactor

90ae9a8

# Conflicts: # pygsp/graphs/nngraphs/nngraph.py # pygsp/tests/test_graphs.py

backport README changes from master

648fa91

Merge branch 'master' of https://github.com/epfl-lts2/pygsp into nn_r…

96fa5f6

…efactor # Conflicts: # README.rst

Merge branch 'master' into nn_refactor

c26e449

add nmslib

8e7c553

test flann when not on windows

b83e467

use the same code to build sparse matrix for knn and radius

28b7858

building the graph with rescale/center=False should also work

188c4a6

Merge pull request #1 from naspert/nmslib

59c131a

Nmslib

update doc for nmslib

8e98b77

nperraud added 2 commits July 22, 2019 10:44

make test work

29d6fa6

fix tests

280ae3c

make k=4 to pass tests

0b1242b

nperraud added 2 commits July 25, 2019 09:45

test

ddc290e

making test pass -- not very clean nn function

9ad6bff

update tests

17e24a7

nperraud and others added 7 commits August 17, 2019 13:31

Merge branch 'master' into naspert-nn_refactor

d933292

small fix

b25b3ba

fix doc

8e2ff0e

fix test

74ea306

fix test

cc78ff2

fix test_graphs

92b6fbb

update reduction

152bfae

gcuendet reviewed Oct 17, 2019

View reviewed changes

gcuendet mentioned this pull request Oct 18, 2019

Radius NN with cKDTree #60

Open

Merge branch 'master' into naspert-nn_refactor

3212350

mdeff mentioned this pull request Nov 9, 2019

Add a graph learning module #57

Open

mdeff mentioned this pull request Mar 19, 2021

Bug in Healpix sampling in pygsp deepsphere/deepsphere-pytorch#7

Closed

Conversation

mdeff commented Feb 25, 2019

Uh oh!

nperraud commented Jul 22, 2019

Uh oh!

mdeff commented Jul 23, 2019

Uh oh!

nperraud commented Jul 23, 2019

Uh oh!

nperraud commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nperraud commented Aug 17, 2019

Uh oh!

nperraud commented Aug 19, 2019

Uh oh!

gcuendet commented Oct 17, 2019

Uh oh!

mdeff commented Oct 17, 2019

Uh oh!

gcuendet Oct 17, 2019

Choose a reason for hiding this comment

Uh oh!

gcuendet Oct 18, 2019

Choose a reason for hiding this comment

Uh oh!

mdeff commented Dec 18, 2020 • edited by nperraud Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nperraud commented Dec 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdeff commented Dec 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nperraud commented Dec 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

nperraud commented Jul 26, 2019 •

edited

Loading

mdeff commented Dec 18, 2020 •

edited by nperraud

Loading

nperraud commented Dec 19, 2020 •

edited

Loading

mdeff commented Dec 19, 2020 •

edited

Loading