improvements to the notebook graph editor

[williamstein]

Made the following improvements to the notebook graph editor:

• Increased size of graph editing window.
• Stopped crashes occurring when the graph editor was called on a graph with fewer than 4 vertices or separate components.
• Added the option to display vertex numbering.
• Added slider that allows the user to adjust vertex size.
• Added slider allowing the user to rotate the graph around the center of the editing window.
• Added buttons allowing the user to center and maximize the graph in the editing window.
• Added a circular layout button.
• Added a spring-electrical force based layout algorithm.

Upstream: Reported upstream. Developers acknowledge bug.

CC: @williamstein @fchapoton

Component: notebook

Keywords: graph editor

Author: Kevin Clark, Radoslav Kirov

Reviewer: Rob Beezer, Dima Pasechnik

Issue created by migration from https://trac.sagemath.org/ticket/8222

[sagetrac-kevinc]

this is a very ugly first draft

[sagetrac-kevinc]

Attachment: 8222.patch.gz

a quick test (gets reverted in following patch)

[sagetrac-kevinc]

Attachment: 8222-part2.patch.gz

cleaned up code, added orientation slider and auto maximize checkbox

[sagetrac-kevinc]

Attachment: 8222-part3.patch.gz

Attachment: 8222-part4.patch.gz

Fixed crashes when calling graph_editor for graphs with multiple components or fewer than 4 vertices, increased graph editor size.

[sagetrac-kevinc]

Author: Kevin Clark

[rkirov]

comment:2

I added 8222.patch but then both 8222-part2.patch and 8222-part3.patch failed. Maybe I am doing something wrong?!?

Kevin, can you make one big patch (here is how to do that http://stackoverflow.com/questions/1224379/exporting-mercurial-patch-against-an-old-revision).

Rado

[rkirov]

comment:3

Alright got it to work. I am uploading a big patch that contains part1,2,3 for the sagenb spkg. Part4 still needs to be applied separately to the sage tree.

Everything looks great, I will post a proper review tomorrow.

[rkirov]

contains part1,2,3 of kevin's patches

[rkirov]

comment:4

Attachment: 8222-big.patch.gz

Great work! I loved the spring-electric layout with auto-max (that's how I imagined it all along). Everything worked as expect, so the patch is basically ready to go. However, since we are making basically an User Interface, I have some comments on the design decisions.

I am mostly guided by the mantra that passing UI decisions to the user is not a solution. We should man up and fix the most useful behavior that 99% of users will use (and maybe add the sliders hidden behind a "tweak" button or skip them all together).

• Correct me if wrong but Live with auto-max, spring-el. and default sliders give exactly what I expect on 99% of the graphs. I suggest we make this default (and get rid or hide everything else).

• Similarly, I see no reason for live behavior that shoots vertices out of screen. Link auto-max along with live. At the same time, when graph is paused (not live) auto-max should be off.

• Numbering for vertices should be off by default (graph theorists seldom number their vertices).

• I see only two useful vertex sizes, one for unnumbered vertices and one for numbered. The vertex size slider can be skipped (or hidder by "tweaks" button).

• Unnumbered vertices should be filled black.

• There should be a warning (use JS confirm) with circle layout, since it can potentially destroy a carefully planned layout and there is no undo.

Rado

[rkirov]

comment:5

I have implemented my vision (all of the comments above) at http://www.math.uiuc.edu/~rkirov2/js-graph-editor/ and also at the repo http://bitbucket.org/radokirov/js-graph-editor .

[rkirov]

Attachment: 8222-sagenb.patch.gz

contains kevin's code, my improvements and the new processing library.

[rkirov]

Attachment: 8222-sage.patch.gz

contains kevin's patch, and my JSON-fication of the graph editor code

[rbeezer]

comment:6

I tried applying 8222-sagenb.patch and 8222-sage.patch to 4.3.4. The "sage" patch applied fine, but I got several failures with the sagenb patch.

Finding no hg repo for the notebook I initiated one where I thought it belonged, at

local/lib/python2.6/site-packages/sagenb-0.7.<something>-py2.6.egg

(I can't recall the version number on the notebook and am not on the right machine).

Do I need to install the development version of the notebook? (I've done this before, the drill with python setup.py -develop......) Or do I need to apply more than just the two patches?

Anyway, any guidance in testing this on a stock Sage install would be welcome.

Thanks,
Rob

[rkirov]

due to complaint I made replacement for sagenb-big patch. Based on sagenb-0.7.5.3

[rkirov]

comment:7

Attachment: 8222-sagenb-mega.patch.gz

Hi Rob,

You are the second person complaining for the sagenb patch so I probably made the wrong patch. So I erased my sagenb folder, pasted the new code and made a new patch. Try 8222-sagenb-mega.patch. It has everything to make the graph_editor look like the one on my website. Also has fix for the loop bug you found, which the old one didn't have.

There is a hg repo for the notebook, you don't need to recreate anything. Just extract and go to /sagenb-0.7.5.3/src/sagenb . Now all your "hg patch" commands should work.

Here is a summary of everything needed to make this run:

1. apply 8222-sage.patch to Sage.
2. untar sagenb (with tar -xvf sagenb-0.7.5.3.spgk)
3. apply 8222-sagenb-mega.patch in sagenb-0.7.5.3/src/sagenb with "hg patch 8222-sagenb-mega.patch"
4. run SAGEPATH/sage -python setup.py -develop in sagenb-0.7.5.3/src/sagenb
5. run sage and enjoy!

Rado

[rbeezer]

comment:8

Hi Rado,

Thanks - those instructions worked just fine for me and it is running nicely. Comments later after some testing.

For anybody else - two minor items - I could only find 0.7.5.1 in the directory specified on the notebook project page (which is what I used), and step (4) doesn't have a dash before "develop".

Rob

[rbeezer]

comment:9

Hi Rado and Kevin,

Been experimenting with this over the weekend - very, very nice. I think I've learned enough about the pieces to be able to review this now.

Were you going to

(1) disable red edges?

(2) turn-off auto-maximize?

One suggestion I have right now (without having done a 100% thorough review) is to perhaps check very early on to see if the input is even a graph? Right now totally wrong input results in a traceback on getting the position dictionary (which is not very informative to the newbie).

Let me know the status of this (ie will you be making more changes?) and I'll see about doing a careful review. I've got some longer-term, broader comments that I'll post to sage-devel right now.

Rob

[sagetrac-pang]

The line defining graph_to_json is duplicate in graph_editor.py, that's all.

[rkirov]

comment:10

Attachment: 8222-removed-duplicate-line.patch.gz

Hi Rob,

I was waiting to see what Kevin has to say (since I basically took his code and tweaked with my personal preferences, without consulting with him). However, I haven't heard back from him. You are right we should add some type checking.

I wanted to remove auto-maximize, but since it has Kevin's original work, I wanted to hear back from him. Are you in favor of removing it? If nobody else, says something, I will just make final patch with:

1. removed red-edges
2. removed buttons for maximize and auto-maximize
3. switch delete to keyboard 'd'

on the sage side:

1. add type checking
2. incorporate pang's patch (thanks for patch, that was very sloppy of me)

Then Rob can review it and have the new (in my opinion much superior version of graph editor) in Sage.

[rkirov]

comment:18

Replying to @williamstein:

I tried this as part of #8725 -- and it doesn't work at all; totally broken. Tim Dumol says the problem is "a missing script". So, needs work.

hmm, weird, it is all working on my setup. I downloaded sagenb-0.8 from #8725, applied rob's all-in-one patch, ran setup.py develop and there was nothing broken.

Can Tim send me some more info on how to reproduce that bug, so I can work on fixing it?

[TimDumol]

comment:19

Actually, I got it working on my machine already without touching anything. William's still having trouble though.

[rkirov]

comment:20

Replying to @TimDumol:

Actually, I got it working on my machine already without touching anything. William's still having trouble though.

Maybe it was a caching issue. Also i have only tested it on Firefox and Chrome, so maybe there is some browser specific issue?

[TimDumol]

comment:21

Since this has been confirmed to work, can this be marked positive review now?

[kcrisman]

comment:22

This was not merged, as far as I can tell. The Sage library patch definitely isn't in, and the end of the upstream sagenb graph_editor.html indicates it wasn't merged there either.

[kcrisman]

Changed merged from sagenb-0.8 to none

[kcrisman]

comment:23

Notice that the graph editor hasn't worked in a long time, but this pull request fixes it!

[fchapoton]

Changed keywords from none to graph editor

[kcrisman]

Upstream: Reported upstream. Developers acknowledge bug.

[kcrisman]

comment:31

See this sagenb issue, in case anyone ever wants to port this to the latest.

[mkoeppe]

comment:32

Proposing to close all sagenb tickets as outdated, so that all remaining open tickets in the notebook component are about the Jupyter notebook.

[dimpase]

Changed reviewer from Rob Beezer to Rob Beezer, Dima Pasechnik