sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.31k stars 449 forks source link

Make graph display configurable #18289

Closed vbraun closed 9 years ago

vbraun commented 9 years ago

CC: @nathanncohen @dcoudert

Component: graph theory

Author: Volker Braun

Branch/Commit: 78a81e2

Reviewer: Nathann Cohen

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

vbraun commented 9 years ago
comment:1

Note that __repr__ did not change:

sage: repr(graphs.RandomGNP(500,.2))
'RandomGNP(500,0.200000000000000): Graph on 500 vertices'

This is only about rich output, which is different from the standard repr.

If you want rich output of graphs to be just text then thats fine with me, but show() and pretty_print() will then also just be text.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:2

I want the graphs to be printed as they were before. If you want to change the behaviour of .show() that will be a one-year deprecation warning, as usual. Please. You are making things very complicated for whoever uses graphs.

Nathann

vbraun commented 9 years ago
comment:3

We don't need deprecation for bugs. show() behaving different for graphs than for everything else is a bug.

You can still get graphical output, you'll just have to call graph.plot() instead of show(graph).

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:4

I am not here to discuss whether show is implemented as it should, Volker. You changed the default output to plot graphs instead of displaying text, and I ask you to please revert that. It is very impractical for us. Please.

Nathann

vbraun commented 9 years ago
comment:5

I said already that I'm happy to do that if thats what you really want.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:6

I do not remember your saying that you would agree to revert those changes. If you did please excuse me, and please do that. Thank you,

Nathann

vbraun commented 9 years ago

Branch: u/vbraun/faster_output_for_large_graphs

vbraun commented 9 years ago

Commit: ea2ccc2

vbraun commented 9 years ago

Description changed:

--- 
+++ 
@@ -1 +1 @@
-Rich output for graphs can be very slow. Suggestion: implement a size cutoff (#of vertices/edges) above which no graphics is produced, or a different kind of graphics than the usual spring-layout graph.
+No rich output for graphs since it might be too helpful for novices.
vbraun commented 9 years ago

New commits:

3fd29a0Remove rich output for graphs
ea2ccc2fix doctests
6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

Author: Volker Braun

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

Description changed:

--- 
+++ 
@@ -1 +1 @@
-No rich output for graphs since it might be too helpful for novices.
+This branch reverts the changes that #17821 made to graph/, so that the default output of a graph in the console stays the usual text-description of the graph
6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

Reviewer: Nathann Cohen

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:10

Thanks,

Nathann

kcrisman commented 9 years ago
comment:11

This is only about rich output, which is different from the standard repr.

For the record, I think that most users will not understand this subtle difference. I know that I don't. What is "rich output", anyway?

That said, if you think it's a pretty nasty bug that show has different results for different object types, that could be worth discussing on sage-devel. I guess for most non-plot objects I can think of, show does some LaTeXing, right? And it seems reasonable to me to have different default outputs for

sage: Obj

for different kinds of objects - a plot should be shown, a symbolic expression should just be printed, a matrix should be printed unless it's too big, etc., while

sage: Obj.show()

should "show" the object in some meaningful way. But maybe that's related to the pretty_print, which I never really used but maybe is becoming more important?

Sorry for this postscript, I just feel like there are some hairs being split here that are perhaps overly infinitesimally split already.

vbraun commented 9 years ago
comment:12

Rich output (_rich_repr_) is a hook for an object to provide more than just text output to the UI.

The functions show and pretty_print display the rich output immediately (like print), without waiting for the current computation to finish.

The graph.show() method does its own thing, though the graph plotting (wtf is GraphPlot) reinvents the wheel in slightly less round ways so often that I have lost count.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:13

The graph.show() method does its own thing, though the graph plotting (wtf is GraphPlot) reinvents the wheel in slightly less round ways so often that I have lost count.

I barely ever touched the file, except to add some doc and handle default options somewhat correctly. If you think that we can do without it by using some more general code I really have no objection to make.

About the difference between show/pretty_print/repr, I have to agree with Karl-Dieter that I find the differences rather thin. Maybe _rich_repr_ makes sense for latex output which would look better in the notebook, and would be impossible to read in the console? For graphs, however, there is no general way to have a 'readable/enlightenning' plot, so the best is probably to not represent it graphically unless requested explicitly.

Nathann

kcrisman commented 9 years ago
comment:14

Anyway, I don't have any skin in this game but just wanted to figure out what the game was even about.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from ea2ccc2 to e9e7759

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

e9e7759fix two more doctests
dcoudert commented 9 years ago
comment:16

Hello,

I arrive after the battle in this ticket since it took me some time to compile beta2. For some unknown reason, make results in the construction of a tout of sage in all languages. A bit too much. I should learn how to use it to speed up compilation.

I'm happy that the automatic show this behavior has been removed. I'm playing with many graphs with (tens of) thousands of nodes, most of the times remotely on a server, and it is very annoying (and long) when the server tries to display an image...

David.

vbraun commented 9 years ago
comment:17

Thats why I proposed a size cutoff. IMHO it is truly helpful to students to see graphics if the graph is small. But then I don't really care that much.

dcoudert commented 9 years ago
comment:18

Here size does not matter. When you are like me working remotely, even small graphs could be a problem. I just had problems to close my ssh connection (from a standard mac book air with yosemite to a linux server). I had to kill it. I don't know what's going on, and I'm not sure I want to know...

I understand that this behavior could be interesting for students working on the notebook, but not at all for me (working only in console mode). So I'm glad it is removed.

David.

vbraun commented 9 years ago
comment:19

Maybe you just want to turn graphics off (%display graphics disable) instead of making your ssh-over-300-baud-modem workflow the model for how a modern UI should operate.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:20

Thats why I proposed a size cutoff. IMHO it is truly helpful to students to see graphics if the graph is small. But then I don't really care that much.

HMmmmmm... Well, I know that in the SMC, William changed the behavior of view(g) so that it is an alias of g.show(method="js"). So somehow it is a bit easier to view graphs.

Nathann

dcoudert commented 9 years ago
comment:21

Replying to @vbraun:

Maybe you just want to turn graphics off (%display graphics disable) instead of making your ssh-over-300-baud-modem workflow the model for how a modern UI should operate.

Sorry for being old-style, but it is very convenient for me to launch computations on a server remotely (ssh and screen), some of them lasting for days. I would really appreciate not to be forced to type extra commands at run time.

David.

novoselt commented 9 years ago
comment:22

I think part of the problem is with putting potentially difficult things into magic methods, which to some extent is fixed for matrices. Rather than having _latex_ and _repr_ produce code for accurate representations, they should call some other method for doing it in simple cases and otherwise return a sensible summary about size/structure. Those who really want to typeset big objects can call those non-magic methods directly.

As for extra commands for researches vs. extra commands for students (and teachers who deal with these students!), I'd say that students are at least as big a target for Sage and the choice is not at all clear. Except that it tends to be way easier to teach extra commands to researches...

vbraun commented 9 years ago
comment:23

Replying to @dcoudert:

Sorry for being old-style, but it is very convenient for me to launch computations on a server remotely (ssh and screen), some of them lasting for days. I would really appreciate not to be forced to type extra commands at run time.

Tough, but IMHO that is not a use case that we should optimize the Sage user interface for at the expense of making it harder to use for novices.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:24

Tough, but IMHO that is not a use case that we should optimize the Sage user interface for at the expense of making it harder to use for novices.

Hey Hey... It is not exactly 'hard to use for novices'. We get a plot of a graph with g.plot(), g.show(), plot(g) and show(g). It is not exactly like we go out of our way to hide stuff :-P

Nathann

edit: show(g) actually prints some LaTeX code. I can't say that I see the point of that, and the choice of 'show' for such a thing is a bit mysterious to me.

videlec commented 9 years ago
comment:25

Note that the picture for

sage: Graph()

was especially helpful! I am glad this gets reverted.

Vincent

vbraun commented 9 years ago
comment:26

Maybe you also want to complain about Graphics() being useless output?

vbraun commented 9 years ago
comment:27

Replying to @nathanncohen:

edit: show(g) actually prints some LaTeX code. I can't say that I see the point of that, and the choice of 'show' for such a thing is a bit mysterious to me.

The show() function prints latex for objects that do not define rich output, under the assumption that you didn't want text output since you could have had that without show().

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:28

The show() function prints latex for objects that do not define rich output, under the assumption that you didn't want text output since you could have had that without show().

Is it a good idea? I mean, having show(<something>) output LaTeX code? We have a latex(<something>) already. Also, 'show me a graph' is probably better answered with a picture than with code that you have to copy/paste in a file, before compiling it.

kcrisman commented 9 years ago
comment:29

Maybe you also want to complain about Graphics() being useless output?

Naturally that is sarcastic, but one can point out that it's hard to imagine someone wanting non-graphical output for a graphic, while for a graph it's not really clear one wants a picture of the graph. I also like

sage: Graphics().show(xmin=-20,xmax=20)

s it a good idea? I mean, having show() output LaTeX code?

I think it outputs LaTeXed output, not the actual code for the LaTeX, just to be clear.

vbraun commented 9 years ago
comment:30

Replying to @nathanncohen:

Is it a good idea? I mean, having show(<something>) output LaTeX code?

As I said, its IMHO not a good idea to not make use of the rich output for graphs. But apparently we can't agree on the time of the day here. Meanwhile, William can make SMC user-friendly. Maybe we should just send all new users there and not have them download free software to their own computers.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:31

As I said, its IMHO not a good idea to not make use of the rich output for graphs. But apparently we can't agree on the time of the day here.

It is very easy to get a graphical representation of a graph in Sage. This being said, we also need the basic feature of "typing the name of an object in order to see what exactly it is". And for this a string description (possibly indicating the name of the graph) is a good thing.

Meanwhile, William can make SMC user-friendly. Maybe we should just send all new users there and not have them download free software to their own computers.

William's work these days is apparently totally dedicated to the user interface problems. He tries to gather money to this aim, tries to hire people to do that, while we implement new mathematical features, fix the bugs, i.e. make this thing good and reliable for what we need. I regret it very often that he takes whatever he needs from us, improves what he likes for his own ends and does not contribute back to the project. He must have found thousands of bugs, imperfections and inconsistencies.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:32

P.S.: Apparently show(graphs.PetersenGraph()) does not always produce the same output: Karl-Dieter sees a png picture, and I only see LaTeX code being printed in the console. Same version of Sage. He runs mac, I run debian.

vbraun commented 9 years ago
comment:33

Without this branch, show(graphs.PetersenGraph()) displays a picture on sage-6.7.beta2. With it there is no picture.

vbraun commented 9 years ago
comment:34

Replying to @nathanncohen:

I regret it very often that he takes whatever he needs from us, improves what he likes for his own ends and does not contribute back to the project.

Did it ever occur to you that he can't do that because whenever one tries to improve output somebody starts a week-long email thread because they don't like more verbose output / it doesn't fit into their workflow of using sage over a super-slow network connection and they can't be bothered to turn graphics off?

kcrisman commented 9 years ago
comment:35

Without this branch, show(graphs.PetersenGraph()) displays a picture on sage-6.7.beta2. With it there is no picture.

Just confirming that I get a graph in 6.5 and 6.7.beta2; I haven't tried this one. Is it possibly platform-dependent, or perhaps dependent upon some env var about how to automatically display pngs or something?

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:36

Volker, if this branch indeed makes show(g) output LaTeX code, then could you revert this change too? It is a regression.

dcoudert commented 9 years ago
comment:37

Replying to @vbraun:

Did it ever occur to you that he can't do that because whenever one tries to improve output somebody starts a week-long email thread because they don't like more verbose output / it doesn't fit into their workflow of using sage over a super-slow network connection and they can't be bothered to turn graphics off?

I didn't know that it was possible to turn graphics off. Is there a way to turn it off by default? Also, when you travel, you frequently have to use super-slow connections or connections with volume constraints (e.g. a few MB per day). You then try to limit the amount of data you transmit. It happens to me last week in Chile, and now I hate automatic system updates.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:38

https://groups.google.com/d/msg/sage-release/C0bw1IA7g58/rqWiN_LdysQJ

vbraun commented 9 years ago
comment:39

Replying to @dcoudert:

I didn't know that it was possible to turn graphics off. Is there a way to turn it off by default?

Sage loads ~/.sage/init.sage on startup, so you just have to put %display graphics disable in there to turn it off by default.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:40

About the speed of plots for graphs: after some profiling it seems that the computation of positions is not at fault. All I see for the moment is that the time is taken by the matplotlib primitives. I'll continue looking.

Volker, could you fix this regression?

Nathann

vbraun commented 9 years ago
comment:41

So whats the preferred behavior:

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:42

So whats the preferred behavior:

  • Show graphs as picture (if the graph is small)
  • Do not participate in rich output, graphs cannot be shown as picture (unless you plot them)
  • Keep the inconsistent behavior bug-for-bug where show(graph) does something different than the displayhook, and leave graphs as the only object in Sage that does not work.

Without addressing the actual implementation, it seems rather clear to me that whoever types show(a_graph) wants a picture of it.

The problem with graphs is that it is potentially very slow to plot them (apparently because of matplotlib), but in general there is not "one good drawing" of a graph. Moreover, at the moment the default plotting algorithm will give you a different drawing every time you call it.

Having a different behaviour depending on the size of the graph does not look good to me either, for it is not like for matrices where you see the whole matrix or just a one-line description: indeed, for graphs it would open a new windows (with a picture) in some cases or print a line of text.

All in all, having show(a_graph) display the TikZ/LaTeX code of the graph is definitely not what we would expect from the command.

Nathann

vbraun commented 9 years ago
comment:43

But when the user types

sage: g = compute_some_graph()
sage: g

then they do not want a picture? The displayhook is just an implicit way of telling the UI to show something.

Unrelated to this ticket, but I agree that we don't want huge tikz output. Maybe the ._latex_() method isn't the right place to spit that out anyways. E.g. open the SageNB notebook, tick the "typeset" checkbox, and then run graphs.RandomGNP(500,.2). Bonus points if you can guess correctly what happens without trying it out (I could not)

videlec commented 9 years ago
comment:44

Replying to @vbraun:

But when the user types

sage: g = compute_some_graph()
sage: g

then they do not want a picture? The displayhook is just an implicit way of telling the UI to show something.

I would say:

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago
comment:45

But when the user types

sage: g = compute_some_graph()
sage: g

then they do not want a picture? The displayhook is just an implicit way of telling the UI to show something.

A plot is just 'one representation' of a graph. Like for posets, for matrices (they have a .plot too). I type 'g' very often just to see whether this variable that is named 'g' in indeed a graph (as you would for any other Python object), but also because it tells me whether that graph allows multiple edges or loops.

This being said, like Vincent I expect that the expected behaviour is different in the notebook. If you want to improve that side of the interface, however, I believe that it might be better to output both in the notebook:

Of course, not being able to plot large graphs is a problem. So like for matrices, perhaps a size limit may be good.. But really, most graphs do NOT look good when you plot them, and the result may appear very very very unprofessional, which is probably something you want to avoid:

sage: graphs.RandomGNP(50,.5).show()

Perhaps the default layout can be slightly improved, but I have no idea how to solve the performance problem with matplotlib.

Unrelated to this ticket, but I agree that we don't want huge tikz output.

Why is that unrelated to this ticket? Didn't you say that this change (from picture to latex output) had been made by #17821?

Bonus points if you can guess correctly what happens without trying it out (I could not)

With this branch applied it gives me the latex code O_o

Nathann