Replace sage.graphs.graph_editor with a call to phitigra

[mkoeppe]

phitigra was added as an optional package in #30540.

To launch the editor:

sage: e = graph_editor()
sage: e.show()             # the editor widget appears

To retrieve the graph:

sage: G = e.get_graph()   # G is a copy of the drawn graph, both can be modified independently

Depends on #30540

CC: @kcrisman @fchapoton @dcoudert @dimpase @jfraymond @novoselt

Component: user interface

Author: Jean-Florent Raymond

Branch/Commit: 0549fa9

Reviewer: Matthias Koeppe, Kwankyu Lee

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

[jfraymond]

Dependencies: #30540

[jfraymond]

Branch: u/gh-jfraymond/replace_graph_editor

[jfraymond]

New commits:

94717ee

ticket 33639: replacing the deprecated graph editor with a call to phitigra

[jfraymond]

Commit: 94717ee

[dcoudert]

comment:3

Could you add in the ticket description what should be done to launch the editor.
I tried in a Jupiter notebook and only got:

graph_editor()
<phitigra.graph_editor.GraphEditor object at 0x1755ab820>

[jfraymond]

comment:4

Yes, I just added it in the description.

It is slightly different than the previous editor: here we define an editor object, that we have to display (with .show()) in order to draw the graph with the mouse. The drawn graph can then be retrieved with the .get_graph() method of the editor object.

The call to graph_editor could directly show the widget, but then I don't know how we would retrieve the drawn graph.

By the way, I am not done with the changes yet, I only pushed the commit so that I can work on this from a different computer.

[kcrisman]

comment:6

Thanks for working on this! The old graph editor was really a fantastic resource we loved to show off, and a good replacement would have been useful even last week for me to advertise.

If there is the possibility of demonstrating/documenting a few (even if not possible to be tested) examples in your coming updates, that would also be great. It is likely that this would be used by people

Also, if we could see whether this works properly in the Sage cell server before it's released in a final version (beta is fine), that would be good, so I'm cc:ing Andrey N.

[jfraymond]

comment:7

Replying to @kcrisman:

Thanks for working on this! The old graph editor was really a fantastic resource we loved to show off, and a good replacement would have been useful even last week for me to advertise.

If there is the possibility of demonstrating/documenting a few (even if not possible to be tested) examples in your coming updates, that would also be great. It is likely that this would be used by people

Also, if we could see whether this works properly in the Sage cell server before it's released in a final version (beta is fine), that would be good, so I'm cc:ing Andrey N.

Hi,

The repository of the editor widget ( https://github.com/jfraymond/phitigra ) has a demo notebook (demo.ipynb) where some of the examples of use are shown.

Currently it is already possible to try the demo notebook after installing the editor package with sage -pip.

During a short period of time it was possible to run it on mybinder https://mybinder.org/v2/gh/jfraymond/phitigra/master?filepath=demo.ipynb but for some reason (on the mybinder side?) it stopped working.

[dcoudert]

comment:8

I tried on macOS 12.2.1 and it works well. It just needs some documentation.

[mkoeppe]

comment:9

+    try:
+        from phitigra import GraphEditor
+    except ModuleNotFoundError:
+        raise ModuleNotFoundError("The graph editor is provided by the optional package phitigra, which currently not installed.")

It's best to replace this with a "lazy_import with feature", similar to
https://github.com/sagemath/sage-prod/blob/develop/src/sage/sat/solvers/picosat.py#L23

[sagetrac-git]

Changed commit from 94717ee to 6323a38

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

6323a38

33639: use of lazy import + examples

[sagetrac-git]

Changed commit from 6323a38 to f2adae6

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

8a58884	33639: minor improvements in documentation
87b67c9	33639: Merge branch 'develop' into replace_graph_editor
f2adae6	33639: added live widget examples in the docstrings with ".. JUPYTER-EXECUTE"

[jfraymond]

Author: Jean-Florent Raymond

[jfraymond]

comment:12

There are now some live examples of the widget using the recently introduced ".. JUPYTER-EXECUTE::" sphinx directive (this is the reason why I merged the develop branch). I was not able to completely test them, presumably because the version of sage running them (online?) does not include the current ticket yet. If this is a problem I can revert to a previous commit and add the examples in an other ticket.

[dcoudert]

comment:13

What's the expected behavior of the live examples if phitigra is not installed ?

[jfraymond]

comment:14

That's a good question. I just tested and the documentation does not build in this case :(.

If I guessed correctly from what I experienced in this first encounter with sphinx-jupyter

• there is a non-interactive "preview" of the jupyter part generated when building the doc (this clearly requires the package to exist on the computer building the doc)
• then one can click on the "Make live" button and run the code of the cell on some server somewhere in the cloud (for this to work this server as well should have the package)

As it is an optional package I guess the correct choice here is to remove these live examples, right ?
By the way, how should I proceed with the doctests (that also require the package to be installed)? Is there a way to tell sage to skip the tests in a file if the package is not present, of is it better to mark all the examples and tests with # not tested? (Tests are being made in the package anyway).

[dcoudert]

comment:15

You can try # optional - phitigra, but since the introduction of Feature other solutions might be better.

Matthias will certainly clarify this.

[mkoeppe]

comment:16

# optional - phitigra is the correct solution, but to make it work, you will have to add a module sage.features.phitigra. You can adapt it from sage.features.igraph.

[mkoeppe]

comment:17

... or adapt it from sage.features.sphinx, which is a tiny bit simpler.

[kwankyu]

comment:18

This is a nice addition to sage. Thanks for working on this.

A few comments:

1. The ticket Rework on live sage documentation #33320 would make all code blocks in sage docoumentation runnable. This will in effect deprecate using .. JUPYTER-EXECUTE:: directly. The ticket needs review too.

2. Please respect the developer's guide: https://doc.sagemath.org/html/en/developer/coding_basics.html#headings-of-sage-library-code-files. The file graph_editor.py is old, and does not follow the guidelines.

[kwankyu]

comment:19

Replying to @kwankyu:

1. The ticket Rework on live sage documentation #33320 would make all code blocks in sage docoumentation runnable. This will in effect deprecate using .. JUPYTER-EXECUTE:: directly. The ticket needs review too.

To be clear, there is no problem in your using .. JUPYTER-EXECUTE:: in this ticket.

[mkoeppe]

comment:25

+from sage.features import PythonModule
+lazy_import('phitigra', 'GraphEditor',
+            feature=PythonModule('phitigra', spkg='phitigra'))

Now that sage.features.phitigra.Phitigra exists, it is best to use it here

[sagetrac-git]

Changed commit from 961ec2f to 8b86a03

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

8b86a03

33639: using sage.phitigra.feature

[jfraymond]

comment:27

I just replaced like that:

-from sage.features import PythonModule
-lazy_import('phitigra', 'GraphEditor',
-            feature=PythonModule('phitigra', spkg='phitigra'))
+from sage.features.phitigra import Phitigra
+lazy_import('phitigra', 'GraphEditor', feature=Phitigra())

[mkoeppe]

Reviewer: Matthias Koeppe, ...

[mkoeppe]

comment:28

Thanks, the use of Features looks good now.

[kwankyu]

Changed reviewer from Matthias Koeppe, ... to Matthias Koeppe, Kwankyu Lee

[kwankyu]

Changed commit from 8b86a03 to 967cd5d

[kwankyu]

Changed branch from u/gh-jfraymond/replace_graph_editor to public/33639

[kwankyu]

New commits:

967cd5d

Reviewer edits

[kwankyu]

comment:30

I did some edits. If you are okay with those, then you can give positive review.

[jfraymond]

comment:31

Thanks!

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

0549fa9

33639: minor change to make the patchbots happy

[sagetrac-git]

Changed commit from 967cd5d to 0549fa9

[fchapoton]

comment:34

this was not necessary ; patchbot is dumb and only giving advice

suggestion : never ever do something on a positive reviewed ticket, unless it is critical

[jfraymond]

comment:35

OK, thanks for the advice and sorry for the useless commit then.
I though this was what was preventing the ticket to be closed.
And indeed the patchbot can be dumb, here I think it was the word "sagenb" in a comment that was causing the problem.

[fchapoton]

comment:36

Bah, ça prend toujours une plombe pour qu'un ticket soit intégré dans sage. Et encore plus longtemps quand on est dans une période juste avant une nouvelle version. La 9.6 ne va pas tarder.

[vbraun]

Changed branch from public/33639 to 0549fa9