Opened 9 years ago

Closed 5 years ago

#13418 closed enhancement (duplicate)

Add method for drawing graphs using Ivan Kuckir graph drawer

Reported by: slabbe Owned by: slabbe
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: graph theory Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: public/ticket/13418 (Commits, GitHub, GitLab) Commit: 77b51abf36b6da2b4b172133d0c2046313b16b39
Dependencies: Stopgaps:

Status badges

Description (last modified by slabbe)

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: 13418_ivank_graph_drawer-sl.patch

Attachments (2)

13418_ivank_graph_drawer-sl.2.patch (3.9 KB) - added by slabbe 9 years ago.
13418_ivank_graph_drawer-sl.patch (3.7 KB) - added by slabbe 8 years ago.
Apply this one only.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 9 years ago by slabbe

  • Status changed from new to needs_review

comment:2 Changed 9 years ago by slabbe

  • Status changed from needs_review to needs_work

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

Changed 9 years ago by slabbe

comment:3 Changed 9 years ago by slabbe

  • Description modified (diff)
  • Status changed from needs_work to needs_review

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!

comment:4 Changed 9 years ago by slabbe

Apply 13418_ivank_graph_drawer-sl.patch

comment:5 Changed 9 years ago by slabbe

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

comment:6 Changed 9 years ago by chapoton

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.

comment:7 Changed 9 years ago by brunellus

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. :-)

comment:8 Changed 9 years ago by chapoton

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.

Changed 8 years ago by slabbe

Apply this one only.

comment:9 Changed 8 years ago by slabbe

  • Description modified (diff)

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!

comment:10 Changed 8 years ago by slabbe

  • Description modified (diff)

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

comment:11 Changed 8 years ago by ncohen

  • Status changed from needs_review to needs_info

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

comment:12 Changed 8 years ago by brunellus

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. :-)

Last edited 8 years ago by brunellus (previous) (diff)

comment:13 Changed 8 years ago by ncohen

Yoooooooooooooooooooooooo !!!

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

Nathann

comment:14 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:15 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:16 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:17 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:18 Changed 6 years ago by chapoton

  • Branch set to public/ticket/13418
  • Commit set to 77b51abf36b6da2b4b172133d0c2046313b16b39

New commits:

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

comment:19 follow-up: Changed 5 years ago by chapoton

  • Milestone changed from sage-6.4 to sage-duplicate/invalid/wontfix
  • Status changed from needs_info to needs_review

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

comment:20 Changed 5 years ago by chapoton

does somebody has any objection against closing this ticket ?

comment:21 Changed 5 years ago by chapoton

  • Status changed from needs_review to positive_review

ok, no reaction, let us close that.

comment:22 Changed 5 years ago by slabbe

No objection.

comment:23 in reply to: ↑ 19 Changed 5 years ago by slabbe

Replying to chapoton:

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

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

comment:24 Changed 5 years ago by vbraun

  • Resolution set to duplicate
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.