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

[WIP] Graph visual #1043

Closed
wants to merge 74 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@lrvdijk
Contributor

lrvdijk commented Aug 3, 2015

This is a new pull request for the upcoming GraphVisual.

Tasks:

  • Create compound visual using ArrowVisual and MarkersVisual
  • Create API for graph layouts
  • Implement Several automatic layouts
    • Random
    • Circular (all nodes on a single circle)
    • Spring (force directed) layout based on the Fruchterman-Reingold algorithm
    • Maybe more
  • Fix layout animation

Issues:

  • Fix arrows below the nodes
@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Aug 3, 2015

Member

@sh4wn note that you can draw inspiration from here for the force-directed stuff:

https://github.com/vispy/vispy/blob/master/examples/demo/scene/force_directed_graph.py

It would be great if you could greatly simplify that example in this PR, by internalizing a bunch of the work. WDYT?

Member

larsoner commented Aug 3, 2015

@sh4wn note that you can draw inspiration from here for the force-directed stuff:

https://github.com/vispy/vispy/blob/master/examples/demo/scene/force_directed_graph.py

It would be great if you could greatly simplify that example in this PR, by internalizing a bunch of the work. WDYT?

Show outdated Hide outdated examples/basics/visuals/graph.py
import sys
import networkx as nx

This comment has been minimized.

@larsoner

larsoner Aug 3, 2015

Member

you'll probably need to add networkx as something that gets installed on Travis for this to pass the example build. Prefer a conda package if it's available, otherwise pip is fine.

@larsoner

larsoner Aug 3, 2015

Member

you'll probably need to add networkx as something that gets installed on Travis for this to pass the example build. Prefer a conda package if it's available, otherwise pip is fine.

Show outdated Hide outdated vispy/visuals/graphs/graph.py
_layouts_map = {
'random': layouts.random,

This comment has been minimized.

@larsoner

larsoner Aug 3, 2015

Member

should probably also have circular?

@larsoner

larsoner Aug 3, 2015

Member

should probably also have circular?

Show outdated Hide outdated examples/basics/visuals/graph.py
def on_draw(self, event):
gloo.clear('black')
self.visual.draw()

This comment has been minimized.

@larsoner

larsoner Aug 3, 2015

Member

you should add a button-press handler that changes the layout type on the fly

@larsoner

larsoner Aug 3, 2015

Member

you should add a button-press handler that changes the layout type on the fly

Show outdated Hide outdated vispy/visuals/graphs/util.py
arrows.extend([
node_coords[reverse[0]],
node_coords[reverse[1]]
])

This comment has been minimized.

@larsoner

larsoner Aug 3, 2015

Member

We will definitely need to vectorize this at some point, it should be possible. But for now it's okay to be explicit :)

@larsoner

larsoner Aug 3, 2015

Member

We will definitely need to vectorize this at some point, it should be possible. But for now it's okay to be explicit :)

This comment has been minimized.

@larsoner

larsoner Aug 3, 2015

Member

(but please add a note that it should be a target of future optimization)

@larsoner

larsoner Aug 3, 2015

Member

(but please add a note that it should be a target of future optimization)

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Aug 3, 2015

Member

Looks like a good start. Let me know when you want additional/deeper feedback.

Member

larsoner commented Aug 3, 2015

Looks like a good start. Let me know when you want additional/deeper feedback.

@lrvdijk

This comment has been minimized.

Show comment
Hide comment
@lrvdijk

lrvdijk Aug 3, 2015

Contributor

Ok, so let's try the rebasing properly with this branch.

What would be the steps to rebase this branch on the current VisPy master?

The procedure I normally follow:

  • git checkout master
  • git pull upstream master
  • git checkout graph-visual
  • git rebase master

After that git status says that the current branch (graph-visual) and the tracking branch (origin/graph-visual) have diverged. This is sort of expected because rebase rewrites commits. But I can't push to that branch anymore (only with -f), so what would be the solution?

Contributor

lrvdijk commented Aug 3, 2015

Ok, so let's try the rebasing properly with this branch.

What would be the steps to rebase this branch on the current VisPy master?

The procedure I normally follow:

  • git checkout master
  • git pull upstream master
  • git checkout graph-visual
  • git rebase master

After that git status says that the current branch (graph-visual) and the tracking branch (origin/graph-visual) have diverged. This is sort of expected because rebase rewrites commits. But I can't push to that branch anymore (only with -f), so what would be the solution?

@lrvdijk

This comment has been minimized.

Show comment
Hide comment
@lrvdijk

lrvdijk Aug 3, 2015

Contributor

Ah thanks already for the review! Yes simplifying that example looks like a nice goal to me!

Contributor

lrvdijk commented Aug 3, 2015

Ah thanks already for the review! Yes simplifying that example looks like a nice goal to me!

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Aug 3, 2015

Member

That's pretty much what I do. When it says there's a conflict, I do:

git mergetool
...
git rebase --continue

Every once in a while, if you have a small commit that e.g. fixes a typo or changes some spacing, and the exact changes exist in master, you'll need to do git rebase --skip instead of git rebase --continue, but that's pretty rare.

I have my system configured so that git mergetool launches meld, an excellent 3-way merge tool. I think it's the default for Ubuntu, actually.

Member

larsoner commented Aug 3, 2015

That's pretty much what I do. When it says there's a conflict, I do:

git mergetool
...
git rebase --continue

Every once in a while, if you have a small commit that e.g. fixes a typo or changes some spacing, and the exact changes exist in master, you'll need to do git rebase --skip instead of git rebase --continue, but that's pretty rare.

I have my system configured so that git mergetool launches meld, an excellent 3-way merge tool. I think it's the default for Ubuntu, actually.

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Aug 3, 2015

Member

Actually, usually I just do this:

git fetch upstream
git rebase upstream/master

That way you don't have to deal with your local master being in some accidental incorrect state, for example if you made a commit there instead of another branch and forgot.

Member

larsoner commented Aug 3, 2015

Actually, usually I just do this:

git fetch upstream
git rebase upstream/master

That way you don't have to deal with your local master being in some accidental incorrect state, for example if you made a commit there instead of another branch and forgot.

@lrvdijk

This comment has been minimized.

Show comment
Hide comment
@lrvdijk

lrvdijk Aug 3, 2015

Contributor

Ok, the rebase went fine, but it's more about this message:

On branch graph-visual
Your branch and 'origin/graph-visual' have diverged,
and have 75 and 19 different commits each, respectively.
  (use "git pull" to merge the remote branch into yours)

Pulling right now from my own origin branch seems strange, or should I just force push it?

Contributor

lrvdijk commented Aug 3, 2015

Ok, the rebase went fine, but it's more about this message:

On branch graph-visual
Your branch and 'origin/graph-visual' have diverged,
and have 75 and 19 different commits each, respectively.
  (use "git pull" to merge the remote branch into yours)

Pulling right now from my own origin branch seems strange, or should I just force push it?

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Aug 3, 2015

Member

You should force push. But before doing that, I'd recommend doing:

git checkout -b graph-visual-bak origin/graph-visual
git checkout graph-visual

Then proceed with

git push origin --force graph-visual

That way you have a local copy of your remote branch in case you accidentally did something wrong.

Member

larsoner commented Aug 3, 2015

You should force push. But before doing that, I'd recommend doing:

git checkout -b graph-visual-bak origin/graph-visual
git checkout graph-visual

Then proceed with

git push origin --force graph-visual

That way you have a local copy of your remote branch in case you accidentally did something wrong.

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Aug 3, 2015

Member

BTW the 19 comes from the 19 commits in this PR, the 75 must be the 19 commits in this PR (that show up as different because the hashes have changed) plus what must be 56 commits that have made it into master since you originally started your branch.

Member

larsoner commented Aug 3, 2015

BTW the 19 comes from the 19 commits in this PR, the 75 must be the 19 commits in this PR (that show up as different because the hashes have changed) plus what must be 56 commits that have made it into master since you originally started your branch.

Show outdated Hide outdated vispy/visuals/graphs/layouts/force_directed.py
self.optimal = 1 / np.sqrt(self.num_nodes)
# make sure we have a LIst of Lists representation
adjacency_mat = adjacency_mat.tolil()

This comment has been minimized.

@lrvdijk

lrvdijk Aug 6, 2015

Contributor

Can I use tolil() here? It's actually a SciPy function, so I should probably check if it's available before actually using the sparse solver.

@lrvdijk

lrvdijk Aug 6, 2015

Contributor

Can I use tolil() here? It's actually a SciPy function, so I should probably check if it's available before actually using the sparse solver.

This comment has been minimized.

@larsoner

larsoner Aug 6, 2015

Member

scipy is a soft dependency, so it would be good to at least check if it's available and throw an error if it's not.

@larsoner

larsoner Aug 6, 2015

Member

scipy is a soft dependency, so it would be good to at least check if it's available and throw an error if it's not.

This comment has been minimized.

@larsoner

larsoner Aug 7, 2015

Member

Actually, you shouldn't need to convert here at all.

@larsoner

larsoner Aug 7, 2015

Member

Actually, you shouldn't need to convert here at all.

This comment has been minimized.

@larsoner

larsoner Aug 7, 2015

Member

...instead, earlier you should ensure that adjacency_mat is either ndarray, or one of the sparse forms that allows row access (maybe any of them, I can't remember?), converting to such a form if necessary e.g. via to_csr().

@larsoner

larsoner Aug 7, 2015

Member

...instead, earlier you should ensure that adjacency_mat is either ndarray, or one of the sparse forms that allows row access (maybe any of them, I can't remember?), converting to such a form if necessary e.g. via to_csr().

@lrvdijk

This comment has been minimized.

Show comment
Hide comment
@lrvdijk

lrvdijk Aug 6, 2015

Contributor

Force directed layout is implemented (with animation, but the animation is not perfect yet).

Top: Vispy
Bottom: NetworkX with Matplotlib

force-directed-vispy
force-directed-nx

Contributor

lrvdijk commented Aug 6, 2015

Force directed layout is implemented (with animation, but the animation is not perfect yet).

Top: Vispy
Bottom: NetworkX with Matplotlib

force-directed-vispy
force-directed-nx

@bollu

This comment has been minimized.

Show comment
Hide comment
@bollu

bollu Aug 6, 2015

Contributor

@sh4wn out of curiosity, what does it look like with AGG enabled for the lines?

Contributor

bollu commented Aug 6, 2015

@sh4wn out of curiosity, what does it look like with AGG enabled for the lines?

Show outdated Hide outdated vispy/visuals/graphs/layouts/force_directed.py
length = np.where(length < 0.01, 0.1, length)
delta_pos = np.transpose(np.transpose(displacement)*t/length)
pos += delta_pos
pos = rescale_layout(pos)

This comment has been minimized.

@lrvdijk

lrvdijk Aug 6, 2015

Contributor

I think this should be changed somehow, but I'm not sure yet how: It normalizes the node coordinates, but each iteration again. This modifies the current iteration values which may not be desirable (networkx only normalizes them at the end)

If anyone knows some other way to make sure the positions stay within [0, 1], please let me know.

@lrvdijk

lrvdijk Aug 6, 2015

Contributor

I think this should be changed somehow, but I'm not sure yet how: It normalizes the node coordinates, but each iteration again. This modifies the current iteration values which may not be desirable (networkx only normalizes them at the end)

If anyone knows some other way to make sure the positions stay within [0, 1], please let me know.

This comment has been minimized.

@larsoner

larsoner Aug 6, 2015

Member

what's the problem? is it too slow? or bad in principle?

@larsoner

larsoner Aug 6, 2015

Member

what's the problem? is it too slow? or bad in principle?

This comment has been minimized.

@lrvdijk

lrvdijk Aug 6, 2015

Contributor

Well, I don't think it slow per se (I don't notice any delays), but it somehow feels a bit wrong to alter the values mid iteration.

Then again, it should scale each point the same with aspect ration maintained, so it don't think it returns incorrect values.

@lrvdijk

lrvdijk Aug 6, 2015

Contributor

Well, I don't think it slow per se (I don't notice any delays), but it somehow feels a bit wrong to alter the values mid iteration.

Then again, it should scale each point the same with aspect ration maintained, so it don't think it returns incorrect values.

This comment has been minimized.

@larsoner

larsoner Aug 6, 2015

Member

You might be able to change/threshold delta_pos instead of changing pos, but that would probably lead to weird behaviors, so rescaling here might be the sanest thing to do.

@larsoner

larsoner Aug 6, 2015

Member

You might be able to change/threshold delta_pos instead of changing pos, but that would probably lead to weird behaviors, so rescaling here might be the sanest thing to do.

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Aug 6, 2015

Member

That example looks nice :)

Member

larsoner commented Aug 6, 2015

That example looks nice :)

@lrvdijk

This comment has been minimized.

Show comment
Hide comment
@lrvdijk

lrvdijk Aug 6, 2015

Contributor

@bollu Not tested yet, because the AGG backend doesn't support the 'segment' connect mode. Which means I would need to create a new LineVisual for each line segment (graph edge). With the OpenGL backend I can pass a big list of vertices and tell it to only draw lines between two consecutive vertices.

Contributor

lrvdijk commented Aug 6, 2015

@bollu Not tested yet, because the AGG backend doesn't support the 'segment' connect mode. Which means I would need to create a new LineVisual for each line segment (graph edge). With the OpenGL backend I can pass a big list of vertices and tell it to only draw lines between two consecutive vertices.

@bollu

This comment has been minimized.

Show comment
Hide comment
@bollu

bollu Aug 6, 2015

Contributor

@sh4wn Ah, all right :)

Contributor

bollu commented Aug 6, 2015

@sh4wn Ah, all right :)

Show outdated Hide outdated vispy/visuals/graphs/layouts/force_directed.py
# update positions
length = np.sqrt((displacement**2).sum(axis=1))
length = np.where(length < 0.01, 0.1, length)
delta_pos = np.transpose(np.transpose(displacement)*t/length)

This comment has been minimized.

@larsoner

larsoner Aug 6, 2015

Member

a few numpy tricks to learn here. First, arr.T is easier to use than np.transpose(arr). Second, you're doing this to get broadcasting, right? You should be able to do this instead:

delta_pos = displacement * t / length[:, np.newaxis]
@larsoner

larsoner Aug 6, 2015

Member

a few numpy tricks to learn here. First, arr.T is easier to use than np.transpose(arr). Second, you're doing this to get broadcasting, right? You should be able to do this instead:

delta_pos = displacement * t / length[:, np.newaxis]

This comment has been minimized.

@lrvdijk

lrvdijk Aug 6, 2015

Contributor

I must say, this almost comes directly from NetworkX (forgot to add attribution in the docs).

But please let me know if things can be improved.

@lrvdijk

lrvdijk Aug 6, 2015

Contributor

I must say, this almost comes directly from NetworkX (forgot to add attribution in the docs).

But please let me know if things can be improved.

This comment has been minimized.

@larsoner

larsoner Aug 6, 2015

Member
@larsoner

larsoner via email Aug 6, 2015

Member
@lrvdijk

This comment has been minimized.

Show comment
Hide comment
@lrvdijk

lrvdijk Aug 7, 2015

Contributor

Ok, with 1000 nodes, it is very slow. The sparse solver is definitely faster, but still takes a lot of time to calculate the new layout. The result is an animation with an FPS of about 0.5 (takes about 2 seconds to calculate the new iteration), or maybe even slower.

I'll see if I can profile this a bit with cProfile.

Contributor

lrvdijk commented Aug 7, 2015

Ok, with 1000 nodes, it is very slow. The sparse solver is definitely faster, but still takes a lot of time to calculate the new layout. The result is an animation with an FPS of about 0.5 (takes about 2 seconds to calculate the new iteration), or maybe even slower.

I'll see if I can profile this a bit with cProfile.

@lrvdijk

This comment has been minimized.

Show comment
Hide comment
@lrvdijk

lrvdijk Aug 7, 2015

Contributor

Profiling shows that the straight_line_vertices function to main reason for the slowdown is, so let's see how to optimize this. :)

Contributor

lrvdijk commented Aug 7, 2015

Profiling shows that the straight_line_vertices function to main reason for the slowdown is, so let's see how to optimize this. :)

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Aug 7, 2015

Member

@sh4wn yes, that function could be much more optimized I think. Is the adjacency matrix sparse, dense, or either?

Member

larsoner commented Aug 7, 2015

@sh4wn yes, that function could be much more optimized I think. Is the adjacency matrix sparse, dense, or either?

@lrvdijk

This comment has been minimized.

Show comment
Hide comment
@lrvdijk

lrvdijk Aug 7, 2015

Contributor

I think it's in general sparse (most nodes only have edges with a few other nodes), but there are definitely cases where there are a lot of edges. But this is just based on a feeling of mine, not really based on data.

I do not yet see how NumPy could be used to create a new vertices list based on an adjacency matrix.

Contributor

lrvdijk commented Aug 7, 2015

I think it's in general sparse (most nodes only have edges with a few other nodes), but there are definitely cases where there are a lot of edges. But this is just based on a feeling of mine, not really based on data.

I do not yet see how NumPy could be used to create a new vertices list based on an adjacency matrix.

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Aug 7, 2015

Member

I mean, is it an instance of scipy.sparse.* or ndarray? Or could it be either of those?

Member

larsoner commented Aug 7, 2015

I mean, is it an instance of scipy.sparse.* or ndarray? Or could it be either of those?

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Aug 7, 2015

Member

And just so the problem is clear, you basically want a fast way to get all the non-zero (i, j) pairs from an ndarray?

Member

larsoner commented Aug 7, 2015

And just so the problem is clear, you basically want a fast way to get all the non-zero (i, j) pairs from an ndarray?

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Aug 7, 2015

Member

If you have an ndarray, you can do:

i, j = np.where(adj_mat)

i then has all the first dimension indices of non-zero values, j all the second dimension indices. The values are then val = adj_mat[i, j].

For a sparse matrix, I'd use to_coo() to get it in a convenient representation.

If you want to treat the upper and lower triangles separately (I assume you do), then you can extract the lower and upper triangles. This will only work for dense (ndarray) matrices. Instead of figuring out how to also do it for coo matrices, it is probably just be easier to make a mask after you have i, j from either method as:

mask = (i < j)

Make sense?

Member

larsoner commented Aug 7, 2015

If you have an ndarray, you can do:

i, j = np.where(adj_mat)

i then has all the first dimension indices of non-zero values, j all the second dimension indices. The values are then val = adj_mat[i, j].

For a sparse matrix, I'd use to_coo() to get it in a convenient representation.

If you want to treat the upper and lower triangles separately (I assume you do), then you can extract the lower and upper triangles. This will only work for dense (ndarray) matrices. Instead of figuring out how to also do it for coo matrices, it is probably just be easier to make a mask after you have i, j from either method as:

mask = (i < j)

Make sense?

Show outdated Hide outdated vispy/visuals/graphs/util.py
Returns a tuple containing containing (`line_vertices`,
`arrow_vertices`)
"""
if adjacency_mat.shape[0] != adjacency_mat.shape[1]:

This comment has been minimized.

@larsoner

larsoner Aug 7, 2015

Member

if adjacency_mat.ndim !=2 or ...

We should also type-check. You might want to put this at the top of the file:

try:
    from scipy.sparse import issparse
except ImportError:
    def issparse(x):
        return False

Then your type-check here can basically be an attempt to cast it to the proper type before dimension checking:

if not issparse(adjacency_mat)) and not isinstance(adjacency_mat, ndarray):
    adjacency_mat = np.array(adjacency_mat, float)
@larsoner

larsoner Aug 7, 2015

Member

if adjacency_mat.ndim !=2 or ...

We should also type-check. You might want to put this at the top of the file:

try:
    from scipy.sparse import issparse
except ImportError:
    def issparse(x):
        return False

Then your type-check here can basically be an attempt to cast it to the proper type before dimension checking:

if not issparse(adjacency_mat)) and not isinstance(adjacency_mat, ndarray):
    adjacency_mat = np.array(adjacency_mat, float)
Show outdated Hide outdated vispy/visuals/graphs/util.py
Notes
-----
Adapted from NetworkX.

This comment has been minimized.

@larsoner

larsoner Aug 7, 2015

Member

make a note that this operates in-place (important for devs to know)

@larsoner

larsoner Aug 7, 2015

Member

make a note that this operates in-place (important for devs to know)

Show outdated Hide outdated vispy/visuals/graphs/util.py
# rescale to (0,scale) in all directions, preserves aspect
for i in range(pos.shape[1]):
pos[:, i] *= scale/lim

This comment has been minimized.

@larsoner

larsoner Aug 7, 2015

Member

Vectorize this function, too. Something like this should work:

pos -= pos.min(axis=1)[:, np.newaxis]
pos *= scale / pos.max()

Make sense? It's very useful in Python / numpy / scipy to start thinking about operations in vectorized form like this. If you don't feel like you could have gotten to this solution on your own, it's worth taking the time to play around with code and data, looking at what's happening along the way, until you do IMO.

@larsoner

larsoner Aug 7, 2015

Member

Vectorize this function, too. Something like this should work:

pos -= pos.min(axis=1)[:, np.newaxis]
pos *= scale / pos.max()

Make sense? It's very useful in Python / numpy / scipy to start thinking about operations in vectorized form like this. If you don't feel like you could have gotten to this solution on your own, it's worth taking the time to play around with code and data, looking at what's happening along the way, until you do IMO.

Show outdated Hide outdated vispy/visuals/graphs/layouts/random.py
def random(adjacency_mat, directed=False):
if adjacency_mat.shape[0] != adjacency_mat.shape[1]:
raise ValueError("Adjacency matrix should be square.")

This comment has been minimized.

@larsoner

larsoner Aug 7, 2015

Member

This check is already done in straight_line_vertices so you don't need it here, right?

@larsoner

larsoner Aug 7, 2015

Member

This check is already done in straight_line_vertices so you don't need it here, right?

Show outdated Hide outdated vispy/visuals/graphs/layouts/random.py
from ..util import straight_line_vertices
def random(adjacency_mat, directed=False):

This comment has been minimized.

@larsoner

larsoner Aug 7, 2015

Member

Need docstring

@larsoner

larsoner Aug 7, 2015

Member

Need docstring

Show outdated Hide outdated vispy/visuals/graphs/layouts/force_directed.py
# enforce minimum distance of 0.01
distance = np.where(distance < 0.01, 0.01, distance)
# the adjacency matrix row
row = np.asarray(adjacency_mat.getrowview(i).toarray())

This comment has been minimized.

@larsoner

larsoner Aug 7, 2015

Member

You might want to write a simple helper to deal with this, like:

def _get_row(adj, idx):
    row = adj[idx]
    if issparse(adj):
        row = row.toarray()
    return row
@larsoner

larsoner Aug 7, 2015

Member

You might want to write a simple helper to deal with this, like:

def _get_row(adj, idx):
    row = adj[idx]
    if issparse(adj):
        row = row.toarray()
    return row
Show outdated Hide outdated vispy/visuals/graphs/layouts/circular.py
Parameters
----------
adjacency_mat : matrix
The graph adjacency matrix

This comment has been minimized.

@larsoner

larsoner Aug 7, 2015

Member

missing directed param

If directed is False, you should say what part of the matrix is used. Just upper-right, lower-left, or the union of the two?

Eventually we might want to allow the adjacency matrix to actually be weights instead of zeros and ones, too, BTW.

@larsoner

larsoner Aug 7, 2015

Member

missing directed param

If directed is False, you should say what part of the matrix is used. Just upper-right, lower-left, or the union of the two?

Eventually we might want to allow the adjacency matrix to actually be weights instead of zeros and ones, too, BTW.

This comment has been minimized.

@lrvdijk

lrvdijk Aug 11, 2015

Contributor

If directed is false, then it uses the whole matrix (for each combination of (i, j) both (i, j) as (j, i) should be non-zero).

Weights is indeed something we should implement.

@lrvdijk

lrvdijk Aug 11, 2015

Contributor

If directed is false, then it uses the whole matrix (for each combination of (i, j) both (i, j) as (j, i) should be non-zero).

Weights is indeed something we should implement.

Show outdated Hide outdated .travis.yml
@@ -102,7 +102,7 @@ install:
# Install numpy, flake
- conda create -n testenv --yes --quiet pip python=$PYTHON > ${REDIRECT_TO};
- source activate testenv > ${REDIRECT_TO};
- conda install --yes --quiet numpy$NUMPY nose pytest > ${REDIRECT_TO};
- conda install --yes --quiet numpy$NUMPY nose pytest networkx > ${REDIRECT_TO};

This comment has been minimized.

@larsoner

larsoner Aug 7, 2015

Member

networkx should only be installed for the DEPS=full build (line 124)

@larsoner

larsoner Aug 7, 2015

Member

networkx should only be installed for the DEPS=full build (line 124)

This comment has been minimized.

@larsoner

larsoner Aug 7, 2015

Member

You will probably need to add both networkx and numpy$NUMPY to the conda install line, too, to make sure networkx doesn't pull in the wrong version via recommendation

@larsoner

larsoner Aug 7, 2015

Member

You will probably need to add both networkx and numpy$NUMPY to the conda install line, too, to make sure networkx doesn't pull in the wrong version via recommendation

This comment has been minimized.

@larsoner

larsoner Aug 7, 2015

Member

...and you'll also need to modify appveyor.yml to add networkx there, too

@larsoner

larsoner Aug 7, 2015

Member

...and you'll also need to modify appveyor.yml to add networkx there, too

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Aug 7, 2015

Member

Looks like you're making good progress, I suspect with the suggested vectorization ideas it will get much faster. Let me know if you need help, we can chat about what's going on on gitter or Google. I really enjoy optimization :)

Member

larsoner commented Aug 7, 2015

Looks like you're making good progress, I suspect with the suggested vectorization ideas it will get much faster. Let me know if you need help, we can chat about what's going on on gitter or Google. I really enjoy optimization :)

@lrvdijk

This comment has been minimized.

Show comment
Hide comment
@lrvdijk

lrvdijk Aug 11, 2015

Contributor

Ok cool, straight_line_vertices is a lot faster now, thanks for the tips :) Most of the code still needs docs, but could you take a look on the NumPy stuff?

With larger networks (1000 nodes) it is still a bit stuttury, about 5 FPS, but this is way better.

Contributor

lrvdijk commented Aug 11, 2015

Ok cool, straight_line_vertices is a lot faster now, thanks for the tips :) Most of the code still needs docs, but could you take a look on the NumPy stuff?

With larger networks (1000 nodes) it is still a bit stuttury, about 5 FPS, but this is way better.

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Aug 11, 2015

Member

There are a lot of numpy tricks, and I only know some of them. I'll see if I can find some others :)

Do you know where the slowdowns are currently?

Member

larsoner commented Aug 11, 2015

There are a lot of numpy tricks, and I only know some of them. I'll see if I can find some others :)

Do you know where the slowdowns are currently?

Show outdated Hide outdated vispy/visuals/graphs/graph.py
if len(kwargs) > 0:
raise TypeError(
"{}.set_data() got invalid keyword arguments: {}".format(

This comment has been minimized.

@larsoner

larsoner Aug 11, 2015

Member

Although .format is stated as the preferred way to do it, it actually seems more common and simpler just to use %s. By convention it's what we use, so go ahead and chenge this.

@larsoner

larsoner Aug 11, 2015

Member

Although .format is stated as the preferred way to do it, it actually seems more common and simpler just to use %s. By convention it's what we use, so go ahead and chenge this.

lrvdijk and others added some commits Aug 13, 2015

Refactor a few function names
* layouts.get -> layouts.get_layout
* PEP8 fixes after _straight_line_vertices rename
Let fruchterman_reingold derive from object
For Python 2 compatibility.
Improve docs of random and circular layout.
Also some small PEP8 fixes.
Improve Fruchterman-Reingold implementation
Now uses NumPy broadcasting instead of using transpose, which is
often a bit faster.
Use the same delta pos calculation method
For both sparse and dense matrices.
Make sure AVAILABLE_LAYOUTS is a tuple
The .keys() function is a generator in Python 3.
Use np.float32 instead of 'f32' as dtype
This fixes the conversion from dense arrays.
Improve check if arrows are empty
Prevents a numpy error if an empty array is given.
Do not index on the pos array
We already use the complete array.
Add initial tests for the graph visual
It currently includes a test for the random and circular layout.
Use np.float32 instead of 'f32'
'f32' is not recognized anymore as proper datatype.
@lrvdijk

This comment has been minimized.

Show comment
Hide comment
@lrvdijk

lrvdijk Oct 19, 2015

Contributor

Hmm why do the tests do not find the networkx module in Travis?

Contributor

lrvdijk commented Oct 19, 2015

Hmm why do the tests do not find the networkx module in Travis?

@QuLogic

This comment has been minimized.

Show comment
Hide comment
@QuLogic

QuLogic Oct 19, 2015

Contributor

The minimal deps build does not (and should not) install networkx. You need to make it a soft dependency.

The full deps builds do find networkx, but are simply failing.

Contributor

QuLogic commented Oct 19, 2015

The minimal deps build does not (and should not) install networkx. You need to make it a soft dependency.

The full deps builds do find networkx, but are simply failing.

@lrvdijk

This comment has been minimized.

Show comment
Hide comment
@lrvdijk

lrvdijk Oct 20, 2015

Contributor

Hmm yes, maybe there are too many factors involved and testing it with a predefined seed is not going to work well..

Contributor

lrvdijk commented Oct 20, 2015

Hmm yes, maybe there are too many factors involved and testing it with a predefined seed is not going to work well..

lrvdijk added some commits Oct 27, 2015

Fix possibility of non-initialized arrow-vbo
Under some conditions self._arrow_vbo was used before is was
actually initialized. This has now been fixed.
Remove fruchterman-reingold layout test
Setting the right pseudo-random-number-generator seeds was not enough
to produce reproducable outputs.
Remove dependency on networkx for layout tests
Use a hardcoded adjacency matrix.
@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Jan 3, 2016

Member

@sh4wn are you done working on this / should I take over?

Member

larsoner commented Jan 3, 2016

@sh4wn are you done working on this / should I take over?

@lrvdijk

This comment has been minimized.

Show comment
Hide comment
@lrvdijk

lrvdijk Jan 19, 2016

Contributor

I'm completely swamped with study assignments right now, and I don't expect to change that in the coming months, sorry! If you would like to take over, please go ahead!

Contributor

lrvdijk commented Jan 19, 2016

I'm completely swamped with study assignments right now, and I don't expect to change that in the coming months, sorry! If you would like to take over, please go ahead!

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner May 19, 2016

Member

@astrofrog do you think we should try to get this in the release? I think I might have time to tweak this to get it mergeable in the next week.

Member

larsoner commented May 19, 2016

@astrofrog do you think we should try to get this in the release? I think I might have time to tweak this to get it mergeable in the next week.

@astrofrog

This comment has been minimized.

Show comment
Hide comment
@astrofrog

astrofrog May 19, 2016

Contributor

@Eric89GXL - I would classify this as 'would be nice but not critical', so if it can't be wrapped up in the next couple of weeks, it's probably not worth holding up the release much longer?

Contributor

astrofrog commented May 19, 2016

@Eric89GXL - I would classify this as 'would be nice but not critical', so if it can't be wrapped up in the next couple of weeks, it's probably not worth holding up the release much longer?

@larsoner larsoner closed this May 19, 2016

@QuLogic

This comment has been minimized.

Show comment
Hide comment
@QuLogic

QuLogic May 19, 2016

Contributor

Did this get replaced?

Contributor

QuLogic commented May 19, 2016

Did this get replaced?

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner
Member

larsoner commented May 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment