Skip to content
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

Add method for drawing graphs using Ivan Kuckir graph drawer #13418

Closed
seblabbe opened this issue Aug 31, 2012 · 25 comments
Closed

Add method for drawing graphs using Ivan Kuckir graph drawer #13418

seblabbe opened this issue Aug 31, 2012 · 25 comments

Comments

@seblabbe
Copy link
Contributor

This adds the following method :

sage: g = graphs.PetersenGraph()
sage: g.g_ivank_string()
'10:1-8,1-9,1-10,2-5,2-7,2-10,3-4,3-7,3-9,4-6,4-10,5-6,5-9,6-8,7-8'

This string can be used for visualization at this adress :

http://g.ivank.net/

Apply: attachment: 13418_ivank_graph_drawer-sl.patch

Component: graph theory

Branch/Commit: public/ticket/13418 @ 77b51ab

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

@seblabbe seblabbe added this to the sage-5.11 milestone Aug 31, 2012
@seblabbe seblabbe self-assigned this Aug 31, 2012
@seblabbe
Copy link
Contributor Author

seblabbe commented Sep 2, 2012

comment:2

oups, the "translation" is not used by the method, will fix it soon.

@seblabbe
Copy link
Contributor Author

seblabbe commented Sep 5, 2012

Attachment: 13418_ivank_graph_drawer-sl.2.patch.gz

@seblabbe
Copy link
Contributor Author

seblabbe commented Sep 5, 2012

comment:3

Oups, I updated the patch but forgot to click the box. Redone it: now both patches are the same.

The patch now works for any graph !

(there might be a limit for the size... I don't know)

Needs review!

@seblabbe

This comment has been minimized.

@seblabbe
Copy link
Contributor Author

seblabbe commented Sep 5, 2012

comment:4

Apply 13418_ivank_graph_drawer-sl.patch

@seblabbe
Copy link
Contributor Author

seblabbe commented Sep 5, 2012

comment:5

A reviewer can also think of a better name for the method...

@fchapoton
Copy link
Contributor

comment:6

Very interesting and useful patch ! This looks good to me. I have some minor comments.

For the name, maybe something like "html_interactive_display" or just "html_show" or maybe "show_interactive", so that the tab completion hints at the method ? You give credit to Ivan Kuckir in the documentation. Maybe an informative and short name is more useful.

Could we ensure that a browser is called when this function is used from a terminal ?

I have a small concern about the time-life of the web site which is called. How long will it exist ?

Anyway, I will give a positive review provided the name has been modified.

@sagetrac-brunellus
Copy link
Mannequin

sagetrac-brunellus mannequin commented Oct 1, 2012

comment:7

Hello!

I like this graph visualisation very much, but the implementation is, I think, a problematic one.

I see no issue with the situation chapoton described (calling the method in a terminal) as user probably do not expect great results from trying to invoke an interactive visualisation inside a terminal. :-) Actually, I do not know if show or plot makes any effort in this direction. It may be a topic for a new Trac ticket.

The problem is, IMHO, the IFRAME -- dependency on some internet server is not good at all, because admins could track our users, because Sage should work without an internet connection as much as possible, and because this internet server do not have to work in future.

So, what to do now? Well, the webpage with the script seems very non-free, the author explicitly claims copyright. There are similar open source solutions on the web that we can use locally (http://arborjs.org/ , for example). I would gladly help with that. Also, you can try to contact Ivan, and ask him if he wants to open the code. Again, I would gladly help with the integration.

Or, you can just ignore me as I am no big authority here. :-)

@fchapoton
Copy link
Contributor

comment:8

Well, plot and show usually work very well from a terminal, by calling either a browser or jmol, for example.

I agree that the dependency to an external website is not a good thing.

@seblabbe
Copy link
Contributor Author

Apply this one only.

@seblabbe
Copy link
Contributor Author

comment:9

Attachment: 13418_ivank_graph_drawer-sl.patch.gz

I agree with you. Returning an iframe that is hidden to the user was not a good idea.

Although there exists other open source alternative for doing such graph visualisation, I believe this patch is simple to code and useful for integration in Sage (I used it myself at least two times to visualize graphs since September). The interaction with other software (closed source or not) is not forbidden in Sage as there exists interfaces to Magma, Maple for instance. An interface to arborjs would be nice as well and I am willing to referee such tickets.

I uploaded a new patch. Instead, I just return an inoffensive string. The user may do whatever he wants with it. I think it is better like that. Before, the earlier code returning html(s) was problematic and prevented code reuse.

Needs review again!

@seblabbe

This comment has been minimized.

@seblabbe
Copy link
Contributor Author

comment:10

Also, I followed the convention that methods returning string representation of a graph ends with "string" :

sage: g = graphs.PetersenGraph()
sage: g.*string*?
g.graph6_string
g.graphviz_string
g.sparse6_string

@seblabbe

This comment has been minimized.

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Dec 15, 2012

comment:11

Helloooooooooo Lukas !!!

I discusse with Sebastien about this patch two days ago, and I didn't get your point about Iframes.. What's the problem with the wesite's owner being able to know who is using his applet ? And possibly also knowing that it is being used through Sage ? We may write to him to ask whether he sees anything wrong with that..

I am a bit against adding more methods and also against adding them when their name is so "mysterious", and gives no idea of what the method actually does... Which is the case with g_ivank_string (I don't believe that anybody who wants to plot a graph would be interested by this method :-P). To me, this patch would make more sense if this functionality could be accessed through an argument to g.show(). There I believe people would find it, with a proper documentaton of course.

Nathann

@sagetrac-brunellus
Copy link
Mannequin

sagetrac-brunellus mannequin commented Dec 15, 2012

comment:12

Hi! I am the last person to block someone else's initiative, so I really do not mind if this pass.

I was trying to make the point that user's graphs are part of his work and shouldn't be shared with anyone else when it isn't both necessary and obvious that this happens. This is weakend by the fact that graph itself doesn't tell much about the nature of your work without the most carefull inspection. And, of course, mathematicians who do actual research on Sage, are not really the target group for this functionality. :-)

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Dec 15, 2012

comment:13

Yoooooooooooooooooooooooo !!!

Oh. Well, then we can add scary warnings everywhere saying that KGB spies may be watching :-)

Nathann

@jdemeyer jdemeyer removed this from the sage-5.11 milestone Aug 13, 2013
@jdemeyer jdemeyer added this to the sage-5.12 milestone Aug 13, 2013
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014
@fchapoton
Copy link
Contributor

Commit: 77b51ab

@fchapoton
Copy link
Contributor

New commits:

45dfc88adding g_ivank_string method to graphs (a html graph drawer format)
77b51abtrac #13418 details (pep8)

@fchapoton
Copy link
Contributor

Branch: public/ticket/13418

@fchapoton
Copy link
Contributor

comment:19

no longer needed, as one can do "g.view(method='js') for a similar experience.

@fchapoton fchapoton removed this from the sage-6.4 milestone Nov 21, 2015
@fchapoton
Copy link
Contributor

comment:20

does somebody has any objection against closing this ticket ?

@fchapoton
Copy link
Contributor

comment:21

ok, no reaction, let us close that.

@seblabbe
Copy link
Contributor Author

comment:22

No objection.

@seblabbe
Copy link
Contributor Author

comment:23

Replying to @fchapoton:

no longer needed, as one can do "g.view(method='js') for a similar experience.

you mean g.show(method='js').

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

No branches or pull requests

4 participants