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: |
Description (last modified by )
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 :
Attachments (2)
Change History (26)
comment:1 Changed 9 years ago by
- Status changed from new to needs_review
comment:2 Changed 9 years ago by
- Status changed from needs_review to needs_work
Changed 9 years ago by
comment:3 Changed 9 years ago by
- 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
Apply 13418_ivank_graph_drawer-sl.patch
comment:5 Changed 9 years ago by
A reviewer can also think of a better name for the method...
comment:6 Changed 9 years ago by
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
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
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.
comment:9 Changed 8 years ago by
- 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
- 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
- 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
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. :-)
comment:13 Changed 8 years ago by
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
- Milestone changed from sage-5.11 to sage-5.12
comment:15 Changed 7 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:16 Changed 7 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:17 Changed 7 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:18 Changed 6 years ago by
- Branch set to public/ticket/13418
- Commit set to 77b51abf36b6da2b4b172133d0c2046313b16b39
comment:19 follow-up: ↓ 23 Changed 5 years ago by
- 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
does somebody has any objection against closing this ticket ?
comment:21 Changed 5 years ago by
- Status changed from needs_review to positive_review
ok, no reaction, let us close that.
comment:22 Changed 5 years ago by
No objection.
comment:23 in reply to: ↑ 19 Changed 5 years ago by
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
- Resolution set to duplicate
- Status changed from positive_review to closed
oups, the "translation" is not used by the method, will fix it soon.