Sage: Ticket #13891: Default parameters for Graph.plot() and Graph.show()
https://trac.sagemath.org/ticket/13891
<p>
Some cleaning there, and some doc, ... and a way for me to define a default size for Graph plots, even if I am not allowed to hardcode it in Sage because of the notebook.
</p>
<p>
Here are the new lines in my init.sage file :
</p>
<pre class="wiki">import sage.graphs.graph_plot
sage.graphs.graph_plot.default_show_options['figsize'] = 15
</pre><p>
Hell Yeahhhhhhhhhhhhhhhhhhh !! <code>:-P</code>
</p>
<p>
Nathann
</p>
<hr />
<p>
Apply to devel/sage
</p>
<ol><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/13891/trac_13891-all_in_one.patch" title="Attachment 'trac_13891-all_in_one.patch' in Ticket #13891">trac_13891-all_in_one.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/13891/trac_13891-all_in_one.patch" title="Download"></a>
</li></ol>en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/13891
Trac 1.1.6ncohenSun, 30 Dec 2012 23:20:54 GMTstatus changed
https://trac.sagemath.org/ticket/13891#comment:1
https://trac.sagemath.org/ticket/13891#comment:1
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
TicketppurkaMon, 31 Dec 2012 10:16:47 GMT
https://trac.sagemath.org/ticket/13891#comment:2
https://trac.sagemath.org/ticket/13891#comment:2
<p>
I think the option could be termed as <code>SHOW_OPTIONS</code> to keep the terminology compatible/similar with what is there in <code>sage.plot.graphics</code>.
</p>
<p>
Also, why did you decide to create another dictionary, when there is already <code>graphplot_options</code> available?
</p>
TicketncohenMon, 31 Dec 2012 12:34:09 GMT
https://trac.sagemath.org/ticket/13891#comment:3
https://trac.sagemath.org/ticket/13891#comment:3
<blockquote class="citation">
<p>
I think the option could be termed as <code>SHOW_OPTIONS</code> to keep the terminology compatible/similar with what is there in <code>sage.plot.graphics</code>.
</p>
</blockquote>
<p>
Really ? <code>O_o</code>
Upper case variables are so ugly...
</p>
<blockquote class="citation">
<p>
Also, why did you decide to create another dictionary, when there is already <code>graphplot_options</code> available?
</p>
</blockquote>
<p>
Well, because the <code>graphplot_options</code> dictionary seems to associate text descriptions to keywords, while I want to associate actual values.
</p>
<p>
Btw, I will try to deal with the pictures of our Belgian evening in Singapour sooner or later <code>:-)</code>
</p>
<p>
Have fuuuuuuuuuuuuuuuuuunn !
</p>
<p>
Nathann
</p>
TicketppurkaMon, 31 Dec 2012 13:01:48 GMT
https://trac.sagemath.org/ticket/13891#comment:4
https://trac.sagemath.org/ticket/13891#comment:4
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13891#comment:3" title="Comment 3">ncohen</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
I think the option could be termed as <code>SHOW_OPTIONS</code> to keep the terminology compatible/similar with what is there in <code>sage.plot.graphics</code>.
</p>
</blockquote>
<p>
Really ? <code>O_o</code>
Upper case variables are so ugly...
</p>
</blockquote>
<p>
Yes, they are ugly. But this acts like a global variable in the file. Also <em>if</em> (and that's a big <em>if</em>, looking at my near future) I get time, I will try to clean up some of the options in plots (<a class="new ticket" href="https://trac.sagemath.org/ticket/13828" title="enhancement: Clean up arguments to all plotting functions (new)">#13828</a>), so I would prefer there are less inconsistencies introduced in new patches! :)
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
Also, why did you decide to create another dictionary, when there is already <code>graphplot_options</code> available?
</p>
</blockquote>
<p>
Well, because the <code>graphplot_options</code> dictionary seems to associate text descriptions to keywords, while I want to associate actual values.
</p>
</blockquote>
<p>
Whoops! Of course! That was so silly of me.
</p>
<blockquote class="citation">
<p>
Btw, I will try to deal with the pictures of our Belgian evening in Singapour sooner or later <code>:-)</code>
</p>
</blockquote>
<p>
Great! By the way that's a nice way to mention Singapore - it is really pouring all day long here!
</p>
<blockquote class="citation">
<p>
Have fuuuuuuuuuuuuuuuuuunn !
</p>
</blockquote>
<p>
You too! Wish you a happy new year!
</p>
<blockquote class="citation">
<p>
Nathann
</p>
</blockquote>
TicketncohenMon, 31 Dec 2012 15:35:57 GMT
https://trac.sagemath.org/ticket/13891#comment:5
https://trac.sagemath.org/ticket/13891#comment:5
<p>
Helloooooooo !!
</p>
<blockquote class="citation">
<p>
Yes, they are ugly.
</p>
</blockquote>
<p>
I am glad we agree on that. Patch updated, ugliness included.
God I hate upper case variables.
And I hate standards even more.
</p>
<blockquote class="citation">
<p>
Great! By the way that's a nice way to mention Singapore - it is really pouring all day long here!
</p>
</blockquote>
<p>
Oops sorry ! French spelling <code>:-)</code>
</p>
<p>
Have fuuuuuuuuuuuuuuuuuuuuuunnn !!
</p>
<p>
Nathann
</p>
TicketnthieryTue, 01 Jan 2013 22:10:50 GMT
https://trac.sagemath.org/ticket/13891#comment:6
https://trac.sagemath.org/ticket/13891#comment:6
<p>
Replying to <a class="closed ticket" href="https://trac.sagemath.org/ticket/13891" title="enhancement: Default parameters for Graph.plot() and Graph.show() (closed: fixed)">ncohen</a>:
</p>
<blockquote class="citation">
<pre class="wiki">import sage.graphs.graph_plot
sage.graphs.graph_plot.default_show_options['figsize'] = 15
</pre></blockquote>
<p>
Gosh, I love that feature!
</p>
<p>
Actually, I would love it even more if it could be set for all plots at once.
</p>
<p>
Happy new year!
</p>
TicketppurkaWed, 02 Jan 2013 10:38:58 GMT
https://trac.sagemath.org/ticket/13891#comment:7
https://trac.sagemath.org/ticket/13891#comment:7
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13891#comment:6" title="Comment 6">nthiery</a>:
</p>
<blockquote class="citation">
<p>
Actually, I would love it even more if it could be set for all plots at once.
</p>
</blockquote>
<p>
Here's a (the?) way
</p>
<pre class="wiki">sage.plot.graphics.Graphics.SHOW_OPTIONS['figsize'] = 15
</pre>
TicketncohenWed, 02 Jan 2013 10:48:21 GMT
https://trac.sagemath.org/ticket/13891#comment:8
https://trac.sagemath.org/ticket/13891#comment:8
<blockquote class="citation">
<p>
Here's a (the?) way
</p>
<pre class="wiki">sage.plot.graphics.Graphics.SHOW_OPTIONS['figsize'] = 15
</pre></blockquote>
<p>
<code>>_<</code>
</p>
<p>
Ok... Perhaps this should be written in the doc somewhere ? <code>:-P</code>
</p>
<p>
Nathann
</p>
TicketncohenWed, 02 Jan 2013 10:54:01 GMT
https://trac.sagemath.org/ticket/13891#comment:9
https://trac.sagemath.org/ticket/13891#comment:9
<p>
By the way, even though this patch lost 100% of its interest for me, I guess it can still be useful at some point for somebody who would like larger nodes or something. So if somebody feels like giving it a review <code>:-P</code>
</p>
<p>
Nathann
</p>
<p>
P.S. I will update it in a couple of seconds to include Punarbasu's (very sensible) remark <code>:-P</code>
</p>
TicketppurkaWed, 02 Jan 2013 10:58:44 GMT
https://trac.sagemath.org/ticket/13891#comment:10
https://trac.sagemath.org/ticket/13891#comment:10
<p>
@ncohen: First of call, thanks for this cleanup! Now, here are some comments:
</p>
<ol><li>there seems to be a doctest failure according to the patchbot.
</li><li>Just a small typo here (dictionary):
<pre class="wiki">+ # This dictinary only contains the options that graphplot
</pre></li><li>Some more typos.. <code>s/explicitely/explicitly</code>
</li><li>About your major change - I will need to look at it more carefully. It seems strange to me that <code>GraphPlot</code> imports <code>Graphics</code> from <code>sage.plot</code> and doesn't make use of the <code>figsize</code> parameter from there. I don't understand why it behaves so.
</li></ol>
TicketncohenWed, 02 Jan 2013 14:02:54 GMTcc changed
https://trac.sagemath.org/ticket/13891#comment:11
https://trac.sagemath.org/ticket/13891#comment:11
<ul>
<li><strong>cc</strong>
<em>vdelecroix</em> added
</li>
</ul>
<p>
Patch updated ! As for the bug, it's yet another combinat side-effect <code>>_<</code>
</p>
<p>
The decorator added a dictionary of default options to all graphs, and some code in this file modifies the dictionary so that *THIS* graph will be well displayed.
</p>
<p>
Vincent : it seems to come from your patch <a class="closed ticket" href="https://trac.sagemath.org/ticket/11422" title="enhancement: modular subgroups (closed: fixed)">#11422</a>. How do you propose that we fix this ?
</p>
<p>
Nathann
</p>
TicketncohenWed, 02 Jan 2013 14:14:49 GMT
https://trac.sagemath.org/ticket/13891#comment:12
https://trac.sagemath.org/ticket/13891#comment:12
<p>
Oh, and there's something else that I would like to know. Does this decorator mean that in order to define a default option for two graphs, you made all Graph objects carry an additional dictionary ?
</p>
<p>
Nathann
</p>
TicketvdelecroixWed, 02 Jan 2013 16:00:45 GMT
https://trac.sagemath.org/ticket/13891#comment:13
https://trac.sagemath.org/ticket/13891#comment:13
<p>
Hi Nathann,
</p>
<blockquote class="citation">
<p>
Vincent : it seems to come from your patch <a class="closed ticket" href="https://trac.sagemath.org/ticket/11422" title="enhancement: modular subgroups (closed: fixed)">#11422</a>. How do you propose that we fix this ?
</p>
</blockquote>
<p>
In the method in Arithgroup.coset_graph() I create a graph and modify the option dictionnary in order that color_by_label is True by default. The reason is that I want a nice picture with
</p>
<pre class="wiki">sage: G = ArithmeticSubgroup_Permutation(S2="(1,2)(3,4)",S3="(1,2,3)")
sage: G.coset_graph().show()
</pre><p>
If you provide another way to initialize the default options to plot/show, just tell me and I can do it or do it yourself. Otherwise, remove the annoying line in ArithmeticSubgroup_Permutation.
</p>
<p>
Cheers,
Vincent
</p>
TicketncohenThu, 03 Jan 2013 15:33:36 GMT
https://trac.sagemath.org/ticket/13891#comment:14
https://trac.sagemath.org/ticket/13891#comment:14
<p>
Well, I guess that can be easily solved this way : would the "options" decorator work if you used it with empty parameters ? Something like that :
</p>
<pre class="wiki">@options()
def plot ....
</pre><p>
This way the dictionary of default arguments would be empty unless you fill it manually, and as a result the general default options from this patch would be used.
</p>
<p>
This being said, this @options decorator has to be mentionned in the documentation of every function to which it is applied. Could you write an explanation of what it does and how it is to be used ? This doc should appear in sage/misc/decorators, toward which the doc of all functions using @option should point so that users know that this feature exists, and how it is to be used.
</p>
<p>
Nathann
</p>
TicketncohenMon, 07 Jan 2013 12:59:54 GMT
https://trac.sagemath.org/ticket/13891#comment:15
https://trac.sagemath.org/ticket/13891#comment:15
<p>
Here it is, with the solution given above. This patch is waiting for a review again <code>:-P</code>
</p>
<p>
Nathann
</p>
TicketncohenMon, 07 Jan 2013 13:00:06 GMTattachment set
https://trac.sagemath.org/ticket/13891
https://trac.sagemath.org/ticket/13891
<ul>
<li><strong>attachment</strong>
set to <em>trac_13891.patch</em>
</li>
</ul>
TicketslabbeThu, 24 Jan 2013 12:11:37 GMT
https://trac.sagemath.org/ticket/13891#comment:16
https://trac.sagemath.org/ticket/13891#comment:16
<p>
I tested the patch. All tests passed. Documentation builds fine. The intended behavior works well for me. Although, here are some remarks below. Mainly, I believe it should be better documented in the doc string of plot and show methods. I also added some questions...
</p>
<ol><li>In the documentation of <code>plot</code> method, I do not understand this:
</li></ol><pre class="wiki">- Default parameters for this method can also be set through the
:class:`~sage.misc.decorators.options` mechanism.
</pre><p>
Can you provide an example instead? like:
</p>
<pre class="wiki">
EXAMPLE:
Default parameters for this method can also be set through the
:class:`~sage.misc.decorators.options` mechanism::
sage: how_should_I_use_the_decoration_mechanism()
</pre><ol start="2"><li>Same comment for <code>show</code> method. I prefer to see an example in the doc which illustrates the utilisation of this default parameters.
</li></ol><ol start="3"><li>Add example in both <code>plot</code> and <code>show</code> like this one:
</li></ol><pre class="wiki">
Create a graph and show it::
sage: g = graphs.PetersenGraph()
sage: g.show()
Change default options for all graphs::
sage: sage.graphs.graph_plot.DEFAULT_PLOT_OPTIONS['edge_color']='red'
Now edges of all graphs are red::
sage: g.show()
sage: g = Graph({3:[4,5]})
sage: g.show()
</pre><ol start="4"><li><code>dictinary</code> still appears
</li></ol><p>
<strong>The following point are just questions...</strong>
</p>
<ol class="upperalpha"><li>Somebody can explain me what is the specification differences between <code>show</code> and <code>plot</code>?
</li></ol><ol class="upperalpha" start="2"><li>Why do you remove the element from the kwds? Is there some efficiency gain?
</li></ol><pre class="wiki">14511 # This dictinary only contains the options that graphplot
14512 # understands. These options are removed from kwds at the same
14513 # time.
14514 plot_kwds = {k:kwds.pop(k) for k in graphplot_options if k in kwds}
</pre><ol class="upperalpha" start="3"><li>Should not <code>{}</code> be the default ? In genereal, this should be a dict. I guess None is fine.
</li></ol><pre class="wiki">"edge_colors" : None,
</pre><ol class="upperalpha" start="4"><li>What is this?
</li></ol><pre class="wiki"> 69 Methods and classes
70 -------------------
71 .. autofunction:: _circle_embedding
72 .. autofunction:: _line_embedding
</pre>
TicketslabbeThu, 24 Jan 2013 12:28:52 GMTreviewer set
https://trac.sagemath.org/ticket/13891#comment:17
https://trac.sagemath.org/ticket/13891#comment:17
<ul>
<li><strong>reviewer</strong>
set to <em>Punarbasu Purkayastha, Sébastien Labbé</em>
</li>
</ul>
TicketppurkaThu, 24 Jan 2013 12:30:56 GMT
https://trac.sagemath.org/ticket/13891#comment:18
https://trac.sagemath.org/ticket/13891#comment:18
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13891#comment:16" title="Comment 16">slabbe</a>:
</p>
<blockquote class="citation">
<ol class="upperalpha"><li>Somebody can explain me what is the specification differences between <code>show</code> and <code>plot</code>?
</li></ol></blockquote>
<p>
<code>plot</code> returns an object of class <code>Graphics</code>. <code>show</code> actually calls matplotlib, renders the <code>Graphics</code> object, and then displays the result. This separation is used when you want to combine one or more graphics on the same figure. You can do the following now
</p>
<pre class="wiki">(graphs.CycleGraph(3).plot() + graphs.CycleGraph(5).plot()).show()
</pre><blockquote class="citation">
<ol class="upperalpha" start="2"><li>Why do you remove the element from the kwds? Is there some efficiency gain?
</li></ol></blockquote>
<p>
This is fine. It is not for efficiency, but to remove the kwds which are not handled by the <code>GraphPlot</code> class.
</p>
<blockquote class="citation">
<ol class="upperalpha" start="3"><li>Should not <code>{}</code> be the default ? In genereal, this should be a dict. I guess None is fine.
</li></ol><pre class="wiki">"edge_colors" : None,
</pre></blockquote>
<p>
I have no opinion on this. As long as it works, I am fine with either of None or {}. :)
</p>
<p>
I have been quite tied up with deadlines lately, so sorry for the delay in reviewing this. Currently, this doesn't apply to 5.6.beta0. I will see if I have an older compiled version that I can use. What I wanted to test and check was whether it is possible to set some value in <code>SHOW_OPTIONS</code> from <code>sage/plot/graphics.py</code> (by using the method I mentioned in <a class="ticket" href="https://trac.sagemath.org/ticket/13891#comment:7" title="Comment 7">comment:7</a>) and have the same value reflected in the <code>GraphPlot</code> code. The last time I had checked this, it didn't work.
</p>
TicketslabbeThu, 24 Jan 2013 14:19:58 GMT
https://trac.sagemath.org/ticket/13891#comment:19
https://trac.sagemath.org/ticket/13891#comment:19
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13891#comment:18" title="Comment 18">ppurka</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13891#comment:16" title="Comment 16">slabbe</a>:
</p>
<blockquote class="citation">
<ol class="upperalpha"><li>Somebody can explain me what is the specification differences between <code>show</code> and <code>plot</code>?
</li></ol></blockquote>
<p>
<code>plot</code> returns an object of class <code>Graphics</code>. <code>show</code> actually calls matplotlib, renders the <code>Graphics</code> object, and then displays the result.
</p>
</blockquote>
<p>
For me, I think the confusion comes from the fact that using plot actually opens the png. I like it if plot returns the Graphics object without opening any png and show returning None and openning the png.
</p>
<p>
Thanks for your answers!
</p>
<p>
I will continue my review (on sage-5.6.rc0 the patch applies fine) and let you check the other SHOW_OPTIONS thing.
</p>
<p>
Sébastien
</p>
TicketppurkaThu, 24 Jan 2013 14:24:11 GMT
https://trac.sagemath.org/ticket/13891#comment:20
https://trac.sagemath.org/ticket/13891#comment:20
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13891#comment:19" title="Comment 19">slabbe</a>:
</p>
<blockquote class="citation">
<p>
For me, I think the confusion comes from the fact that using plot actually opens the png. I like it if plot returns the Graphics object without opening any png and show returning None and openning the png.
</p>
</blockquote>
<p>
Actually, plot never returns the rendered graphics object. What happens when you <em>don't</em> assign any variable to plot like
</p>
<pre class="wiki">a = plot(...) # assigning the output of plot to variable a
</pre><p>
then the <code>_repr_()</code> method of the <code>Graphics</code> class is called. This is similar to the way the <code>print</code> function in Sage works. The <code>_repr_</code> method then implicitly calls show and your graph gets rendered and displayed.
</p>
TicketslabbeThu, 24 Jan 2013 14:38:29 GMT
https://trac.sagemath.org/ticket/13891#comment:21
https://trac.sagemath.org/ticket/13891#comment:21
<p>
One more point. This does not affect the result of plot:
</p>
<pre class="wiki">sage: import sage.graphs.graph_plot
sage: g = graphs.PetersenGraph()
sage: sage.graphs.graph_plot.DEFAULT_SHOW_OPTIONS['figsize'] = 20
sage: g.plot()
sage: sage.graphs.graph_plot.DEFAULT_SHOW_OPTIONS['figsize'] = 2
sage: g.plot()
</pre><p>
One may say it is because I should use <code>DEFAULT_PLOT_OPTIONS</code> instead. But it is not the case. As the following is not affected by changing <code>DEFAULT_PLOT_OPTIONS</code> :
</p>
<pre class="wiki">sage: import sage.graphs.graph_plot
sage: g = graphs.PetersenGraph()
sage: sage.graphs.graph_plot.DEFAULT_PLOT_OPTIONS['figsize'] = 2
sage: g.plot()
sage: sage.graphs.graph_plot.DEFAULT_PLOT_OPTIONS['figsize'] = 20
sage: g.plot()
</pre><p>
Hence, I believe the documentation at the start of the file <code>graph_plot.py</code> should contain at least one example using <code>DEFAULT_PLOT_OPTIONS</code> and one example using <code>DEFAULT_SHOW_OPTIONS</code>. As a user I want to understand why there are two such dict and which one I should use.
</p>
TicketslabbeThu, 24 Jan 2013 14:45:57 GMT
https://trac.sagemath.org/ticket/13891#comment:22
https://trac.sagemath.org/ticket/13891#comment:22
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13891#comment:20" title="Comment 20">ppurka</a>:
</p>
<blockquote class="citation">
<p>
The <code>_repr_</code> method then implicitly calls show and your graph gets rendered and displayed.
</p>
</blockquote>
<p>
So, I believe this is the reason of my confusion. Do we really want a string representation function like <code>_repr_</code> to display a png? If I think about it, I don't like it! I prefer the string representation to tell me that the <code>GraphicObject</code> is made of 345 primitive graphics objects. And then, I decide whether I show it or not...
</p>
TicketppurkaThu, 24 Jan 2013 15:21:45 GMT
https://trac.sagemath.org/ticket/13891#comment:23
https://trac.sagemath.org/ticket/13891#comment:23
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13891#comment:22" title="Comment 22">slabbe</a>:
</p>
<blockquote class="citation">
<p>
So, I believe this is the reason of my confusion. Do we really want a string representation function like <code>_repr_</code> to display a png? If I think about it, I don't like it! <strong>I prefer the string representation</strong> to tell me that the <code>GraphicObject</code> is made of 345 primitive graphics objects. And then, I decide whether I show it or not...
</p>
</blockquote>
<p>
You can do so too :) Just run <code>show_default(False)</code> and you won't get those rendered plots.
</p>
TicketncohenThu, 24 Jan 2013 17:16:08 GMT
https://trac.sagemath.org/ticket/13891#comment:24
https://trac.sagemath.org/ticket/13891#comment:24
<p>
Hellooooooooo Sebastien !
</p>
<p>
I can now answer your many comments, and write down what we said orally across the table <code>:-)</code>
</p>
<p>
1/2) I fixed this doc, but only where it is related to what this ticket implements, and not related to the <code>"@options"</code> decorator. This has to be documented where the decorator is defined, not in these files.
</p>
<p>
3) There is a link from the documentation of show toward graph_plot
</p>
<p>
4) Fixed
</p>
<p>
D) Because I want those two methods to appear in the doc, even if they begin with a <code>_</code>
</p>
<p>
What this new patch does :
</p>
<p>
1) It changes the documentation of <a class="missing wiki">GraphPlot?</a> so that the list of admissible keywords appear only once, and in the documentation.
2) The <a class="missing wiki">GraphPlot?</a>.plot() method accepts any options, and ignores them. I remove the <code>**kwds</code> in its definition, and remove the long description of its possible parameters
3) I remove two default arguments of Poset.plot() which were ignored
4) There's a dprecation warning when a wrong keyword is used
</p>
<p>
Tell me what you think of it !
</p>
<p>
Nathann
</p>
TicketncohenThu, 24 Jan 2013 17:16:22 GMTattachment set
https://trac.sagemath.org/ticket/13891
https://trac.sagemath.org/ticket/13891
<ul>
<li><strong>attachment</strong>
set to <em>trac_13891-second_pass.patch</em>
</li>
</ul>
TicketslabbeThu, 24 Jan 2013 18:51:41 GMT
https://trac.sagemath.org/ticket/13891#comment:25
https://trac.sagemath.org/ticket/13891#comment:25
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13891#comment:23" title="Comment 23">ppurka</a>:
</p>
<blockquote class="citation">
<p>
You can do so too :) Just run <code>show_default(False)</code> and you won't get those rendered plots.
</p>
</blockquote>
<p>
Great!!! Thanks a lot for this trick!
</p>
TicketslabbeThu, 24 Jan 2013 23:55:16 GMT
https://trac.sagemath.org/ticket/13891#comment:26
https://trac.sagemath.org/ticket/13891#comment:26
<p>
Great for the second patch. I like the improvements in the documentation. However, here are some issues:
</p>
<ol><li>The following used to work, but is broken by the actual patches. A deprecation warning is needed :
</li></ol><pre class="wiki">sage: g = graphs.PetersenGraph()
sage: p = g.graphplot()
sage: p.plot(my_favorite_useless_option_I_always_used_in_my_code=45)
Traceback (most recent call last):
...
TypeError: plot() got an unexpected keyword argument 'my_favorite_useless_option_I_have_used_in_my_code'
</pre><ol start="2"><li>(Suggestion) I don't know of another way of sending warning to the user, but in our case it is not really a deprecation warning. Because for instance <code>'scaling_term'</code> was used as an argument by some code and was not used if I understand correctly. Hence, there will be no deprecation as it was not used and will still be unused. Therefore, I suggest to improve the message like this:
</li></ol><div class="wiki-code"><div class="code"><pre> from sage.misc.superseded import deprecation
<span class="gd">-deprecation(13891, (str(opt)+" is not a registered argument for"+
- "GraphPlot ! Please tell us that if it "+
- "should be by writing to sage-devel !"))
</span><span class="gi">+deprecation(13891, (str(opt)+" is not a registered argument for "
+ "GraphPlot and never was! Please tell us that if it "
+ "should be by writing to sage-devel ! "
+ "In a future version of Sage, it might raise "
+ "an error instead so you should fix it."))
</span></pre></div></div><ol start="3"><li>(Remark) BTW, the <code>+</code> are not necessary above. For instance:
</li></ol><pre class="wiki">sage: ('first line'
....: 'second line\n'
....: 'third line')
'first linesecond line\nthird line'
</pre><ol start="4"><li>There are some errors reported by the buildbot:
</li></ol><pre class="wiki"> sage -t -force_lib devel/sage-13891/sage/graphs/generic_graph.py # 1 doctests failed
sage -t -force_lib devel/sage-13891/sage/combinat/posets/posets.py # 2 doctests failed
sage -t -force_lib devel/sage-13891/sage/combinat/cluster_algebra_quiver/quiver.py # 1 doctests failed
sage -t -force_lib devel/sage-13891/sage/combinat/cluster_algebra_quiver/quiver_mutation_type.py # 1 doctests failed
sage -t -force_lib devel/sage-13891/sage/rings/semirings/non_negative_integer_semiring.py # 1 doctests failed
sage -t -force_lib devel/sage-13891/sage/categories/coxeter_groups.py # 1 doctests failed
sage -t -force_lib devel/sage-13891/sage/categories/finite_coxeter_groups.py # 3 doctests failed
</pre>
TicketslabbeThu, 24 Jan 2013 23:56:44 GMTstatus changed
https://trac.sagemath.org/ticket/13891#comment:27
https://trac.sagemath.org/ticket/13891#comment:27
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
TicketncohenFri, 25 Jan 2013 10:22:27 GMTstatus changed
https://trac.sagemath.org/ticket/13891#comment:28
https://trac.sagemath.org/ticket/13891#comment:28
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Updaaaaaaaaaated ! <code>:-P</code>
</p>
<p>
Nathann
</p>
TicketppurkaSun, 27 Jan 2013 12:18:47 GMTdescription changed
https://trac.sagemath.org/ticket/13891#comment:29
https://trac.sagemath.org/ticket/13891#comment:29
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/13891?action=diff&version=29">diff</a>)
</li>
</ul>
<p>
This started out as a small patch, but it became quite big already! Anyway, my original fears are all gone. Things seem to work just fine :) I have just a bunch of minor comments now (waiting for the documentation to build):
</p>
<ol><li>There is a doctest error
<pre class="wiki">File "/home/punarbasu/Installations/sage-5.7.beta0/devel/sage-main/sage/graphs/generic_graph.py", line 13803:
sage: list(sorted(layout_options.iteritems()))
Expected:
[('by_component', 'Whether to do the spring layout by connected component -- a boolean.'),
('dim', 'The dimension of the layout -- 2 or 3.'),
('heights', 'A dictionary mapping heights to the list of vertices at this height.'),
('iterations', 'The number of times to execute the spring layout algorithm.'),
('layout', 'A layout algorithm -- one of "acyclic", "circular", "ranked", "graphviz", "planar", "spring", or "tree".'),
('prog', 'Which graphviz layout program to use -- one of "circo", "dot", "fdp", "neato", or "twopi".'),
('save_pos', 'Whether or not to save the computed position for the graph.'),
('spring', 'Use spring layout to finalize the current layout.'),
('tree_orientation', 'The direction of tree branches -- "up" or "down".'),
('tree_root', 'A vertex designation for drawing trees.')]
Got:
[('by_component', 'Whether to do the spring layout by connected component -- a boolean.'), ('dim', 'The dimension of the layout -- 2 or 3.'), ('heights', 'A dictionary mapping heights to the list of vertices at this height.'), ('iterations', 'The number of times to execute the spring layout algorithm.'), ('layout', 'A layout algorithm -- one of : "acyclic", "circular" (plots the graph with vertices evenly distributed on a circle), "ranked", "graphviz", "planar", "spring"
(traditional spring layout, using the graph\'s current positions as initial positions), or "tree" (the tree will be plotted in levels, depending on minimum distance for the root).'), ('prog', 'Which graphviz layout program to use -- one of
"circo", "dot", "fdp", "neato", or "twopi".'), ('save_pos', 'Whether or not to save the computed position for the graph.'), ('spring', 'Use spring layout to finalize the current layout.'), ('tree_orientation', 'The direction of tree branches -- "up" or "down".'), ('tree_root', 'A vertex designation for drawing trees. a vertex of the tree to be used as the root for the ``layout="tree"`` option. If
no root is specified, then one is chosen at random. Ignored unless ``layout=\'tree\'``')]
</pre></li><li>I think the following should mention something like "<code>You may want to give it as an argument to graphplot() instead</code>" (i.e. the <code>GraphPlot</code> should be all lower case)
<pre class="wiki">865 deprecation(13891, "This method takes no argument ! You may want "
866 "to give it to GraphPlot instead.")
</pre></li></ol>
TicketncohenMon, 28 Jan 2013 14:51:35 GMT
https://trac.sagemath.org/ticket/13891#comment:30
https://trac.sagemath.org/ticket/13891#comment:30
<p>
Patch updated <code>:-)</code>
</p>
<p>
Nathann
</p>
TicketncohenMon, 28 Jan 2013 14:51:49 GMTattachment set
https://trac.sagemath.org/ticket/13891
https://trac.sagemath.org/ticket/13891
<ul>
<li><strong>attachment</strong>
set to <em>trac_13891-third_pass.patch</em>
</li>
</ul>
TicketppurkaMon, 28 Jan 2013 16:33:19 GMT
https://trac.sagemath.org/ticket/13891#comment:31
https://trac.sagemath.org/ticket/13891#comment:31
<p>
I saw a few more documentation problems that are a leftover from your earlier patches. Noticing this only now - sorry <code>:-/</code>
The following should be <code>GenericGraph.graphplot</code> since plot doesn't take any options now.
</p>
<pre class="wiki">29 Here is the list of options accepted by :meth:`GraphPlot.plot` and the
79 This module defines two dictionaries containing default options for the
80 :meth:`~GraphPlot.plot` and :meth:`~GraphPlot.show` methods. These two
29 This module defines two dictionaries containing default options for the
30 :meth:`~GraphPlot.plot` and :meth:`~GraphPlot.show` methods. Obviously, these
</pre><p>
Shouldn't this be changed now? Now, graph.plot() doesn't take anything.
</p>
<pre class="wiki"> def graphplot(self, **options):
"""
Returns a GraphPlot object.
EXAMPLES:
Creating a graphplot object uses the same options as graph.plot()::
</pre><p>
Maybe write it like this?
</p>
<pre class="wiki"> def graphplot(self, **options):
"""
Returns a GraphPlot object.
See :mod:`the documentation of the graph_plot module<sage.graphs.graph_plot>` for
information on default values of this method.
EXAMPLES::
</pre>
TicketncohenTue, 29 Jan 2013 10:56:21 GMT
https://trac.sagemath.org/ticket/13891#comment:32
https://trac.sagemath.org/ticket/13891#comment:32
<p>
Hellooooooooooo !!!
</p>
<p>
Well, why is there a graphplot() method in graphs anyway ? .graphplot and .plot seem to do the same. Shouldn't we remove one of them ?
</p>
<p>
Nathann
</p>
TicketslabbeTue, 29 Jan 2013 11:03:53 GMT
https://trac.sagemath.org/ticket/13891#comment:33
https://trac.sagemath.org/ticket/13891#comment:33
<pre class="wiki">graphplot = deprecated_function_alias(13891, plot)
</pre><p>
or just
</p>
<pre class="wiki">graphplot = plot
</pre>
TicketncohenTue, 29 Jan 2013 11:05:22 GMT
https://trac.sagemath.org/ticket/13891#comment:34
https://trac.sagemath.org/ticket/13891#comment:34
<p>
Couldn't we just remove useless stuff and stop spending a lifetime deprecating it ? ...<code>-_-</code>
</p>
<p>
Nathann
</p>
TicketslabbeTue, 29 Jan 2013 11:15:08 GMT
https://trac.sagemath.org/ticket/13891#comment:35
https://trac.sagemath.org/ticket/13891#comment:35
<p>
One of the main reason why people at LaCIM in Montreal were hating Maple before Sage existed was the fact that their code working in Maple 5 was not working anymore with future releases of Maple...
</p>
<p>
We should try to avoid this with Sage.
</p>
TicketppurkaTue, 29 Jan 2013 11:27:55 GMT
https://trac.sagemath.org/ticket/13891#comment:36
https://trac.sagemath.org/ticket/13891#comment:36
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13891#comment:33" title="Comment 33">slabbe</a>:
</p>
<blockquote class="citation">
<pre class="wiki">graphplot = plot
</pre></blockquote>
<p>
I am actually in favor of this (actually <code>plot = graphplot</code>). Just make this alias so that current stuff doesn't break. Moreover, it will still remain consistent with the other commands and with the <code>plot</code> command from <code>sage.plot</code>.
</p>
<p>
If you remove graphplot, you should revert all the warnings and deprecation that was introduced in plot. As of now, I would say, keep both and make <code>plot = graphplot</code>.
</p>
TicketncohenTue, 29 Jan 2013 12:27:05 GMT
https://trac.sagemath.org/ticket/13891#comment:37
https://trac.sagemath.org/ticket/13891#comment:37
<blockquote class="citation">
<p>
One of the main reason why people at LaCIM in Montreal were hating Maple before Sage existed was the fact that their code working in Maple 5 was not working anymore with future releases of Maple...
</p>
<p>
We should try to avoid this with Sage.
</p>
</blockquote>
<p>
I hate having the graph files stuffed with useless stuff that we keep to prevent two guys in the world from having to do a google search.
</p>
<p>
I'm updating the patch with a deprecation warning.
</p>
<p>
Nathann
</p>
TicketncohenTue, 29 Jan 2013 12:29:33 GMT
https://trac.sagemath.org/ticket/13891#comment:38
https://trac.sagemath.org/ticket/13891#comment:38
<blockquote class="citation">
<p>
I am actually in favor of this (actually <code>plot = graphplot</code>).
</p>
</blockquote>
<p>
Why would we keep graphplot and throw plot away ? plot seems much more natural than graphplot...
</p>
<blockquote class="citation">
<p>
Just make this alias so that current stuff doesn't break.
</p>
</blockquote>
<p>
I will just have to change the calls to graphplot to plot.
</p>
<p>
Tell me if there is anything wrong with that. If you think that it is fine I will begin fixing the doctests.
</p>
<blockquote class="citation">
<p>
Moreover, it will still remain consistent with the other commands and with the <code>plot</code> command from <code>sage.plot</code>.
</p>
</blockquote>
<p>
What do you mean ?
</p>
<blockquote class="citation">
<p>
If you remove graphplot, you should revert all the warnings and deprecation that was introduced in plot.
</p>
</blockquote>
<p>
Why ? plot does not call graphplot !
</p>
<blockquote class="citation">
<p>
As of now, I would say, keep both and make <code>plot = graphplot</code>.
</p>
</blockquote>
<p>
Whyyyyyyyyyyyyyyyyyyy ? Why should we keep two functions that do the same thing ? <code>O_o</code>
</p>
<p>
Nathann
</p>
TicketncohenTue, 29 Jan 2013 12:59:54 GMT
https://trac.sagemath.org/ticket/13891#comment:39
https://trac.sagemath.org/ticket/13891#comment:39
<p>
Hey, I just noticed that .graphplot and .plot just do not do the same thing.
</p>
<p>
graphplot :
</p>
<pre class="wiki">return GraphPlot(graph=self, options=options)
</pre><p>
plot :
</p>
<pre class="wiki">return GraphPlot(graph=self, options=options).plot()
</pre><p>
So it does not really make sense to deprecate one by making it an alias to the other one !
</p>
<p>
The question is : do we need a graphplot method, and what for (knowing that it takes two lines to create the graphplot object from the graph) ? If we do then it stays, if we don't we remove it, or add whatever deprecation warning you may want, but we can't make it an alias of anything.
</p>
<p>
Nathann
</p>
TicketncohenTue, 29 Jan 2013 13:17:57 GMTattachment set
https://trac.sagemath.org/ticket/13891
https://trac.sagemath.org/ticket/13891
<ul>
<li><strong>attachment</strong>
set to <em>trac_13891-fourth_pass.patch</em>
</li>
</ul>
TicketncohenTue, 29 Jan 2013 13:21:05 GMT
https://trac.sagemath.org/ticket/13891#comment:40
https://trac.sagemath.org/ticket/13891#comment:40
<p>
(I just updated the patch so that graphplot stays as it was, along with plot. The references to <code>GraphPlot.plot</code> have been replaced by <code>GenericGraph.plot</code>, which takes all kind of arguments.)
</p>
<p>
Nathann
</p>
TicketslabbeTue, 29 Jan 2013 20:27:56 GMT
https://trac.sagemath.org/ticket/13891#comment:41
https://trac.sagemath.org/ticket/13891#comment:41
<p>
If we keep both (I believe it is good to keep both), I suggest to write the code of the plot method like this, so that it is clear what is the difference and relation between the two :
</p>
<div class="wiki-code"><div class="code"><pre><span class="gd">- return GraphPlot(graph=self, options=options).plot()
</span><span class="gi">+ return self.graphplot(options=options).plot()
</span></pre></div></div>
TicketslabbeTue, 29 Jan 2013 20:31:06 GMT
https://trac.sagemath.org/ticket/13891#comment:42
https://trac.sagemath.org/ticket/13891#comment:42
<p>
There are still two failling tests in the recent buildbot:
</p>
<pre class="wiki">sage -t -force_lib devel/sage-13891/sage/combinat/cluster_algebra_quiver/quiver.py # 1 doctests failed
sage -t -force_lib devel/sage-13891/sage/combinat/cluster_algebra_quiver/quiver_mutation_type.py # 1 doctests failed
</pre>
TicketppurkaWed, 30 Jan 2013 07:58:28 GMT
https://trac.sagemath.org/ticket/13891#comment:43
https://trac.sagemath.org/ticket/13891#comment:43
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13891#comment:40" title="Comment 40">ncohen</a>:
</p>
<blockquote class="citation">
<p>
(I just updated the patch so that graphplot stays as it was, along with plot. The references to <code>GraphPlot.plot</code> have been replaced by <code>GenericGraph.plot</code>, which takes all kind of arguments.)
</p>
<p>
Nathann
</p>
</blockquote>
<p>
Thanks. This is a huge mess.
</p>
<p>
I understand why the plot method is needed. If you look at the code for <code>plot(f)</code> (the generic plot function) then, it returns the output <code>f.plot()</code> method if it is present. I think the current patch is OK. I also think slabbe is right in <a class="ticket" href="https://trac.sagemath.org/ticket/13891#comment:41" title="Comment 41">comment:41</a>.
</p>
<p>
I have combined all the patches and made the <a class="ext-link" href="https://github.com/ppurka/sage-patches/commit/a085570087caed97ee8cc9a2123438f1d6f0e34b"><span class="icon"></span>2-line change</a> to <code>GenericGraph.plot</code>.
</p>
<p>
@slabbe - if you have no other problems with this patch, then you can set it to positive review.
</p>
TicketppurkaWed, 30 Jan 2013 07:59:35 GMTdescription changed
https://trac.sagemath.org/ticket/13891#comment:44
https://trac.sagemath.org/ticket/13891#comment:44
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/13891?action=diff&version=44">diff</a>)
</li>
</ul>
<p>
Patchbot apply trac_13891-all_in_one.patch
</p>
TicketslabbeWed, 30 Jan 2013 11:33:53 GMTstatus changed
https://trac.sagemath.org/ticket/13891#comment:45
https://trac.sagemath.org/ticket/13891#comment:45
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<ol><li>show method of a graph doesn't do anything now. There is a line missing in the code.
</li></ol><ol start="2"><li>2 doctests are still failing (see <a class="ticket" href="https://trac.sagemath.org/ticket/13891#comment:42" title="Comment 42">Comment 42</a>)
</li></ol><p>
<strong>Questions.</strong>
</p>
<ol class="upperalpha"><li>Sorry to ask this question, but why is figsize not an option for <code>plot</code> again?
</li></ol><ol class="upperalpha" start="2"><li>If I understand correctly,
</li></ol><ul><li><code>DEFAULT_SHOW_OPTIONS</code> will affect <code>g.show()</code>.
</li><li><code>DEFAULT_PLOT_OPTIONS</code> will affect <code>g.plot()</code>.
</li></ul><blockquote>
<p>
but
</p>
</blockquote>
<ul><li><code>DEFAULT_SHOW_OPTIONS</code> will not affect <code>g.plot()</code>.
</li><li><code>DEFAULT_PLOT_OPTIONS</code> will affect <code>g.show()</code>.
</li></ul><blockquote>
<p>
Right?
</p>
</blockquote>
<p>
<strong>Comments.</strong>
</p>
<ol class="upperalpha" start="3"><li>This works:
</li></ol><pre class="wiki"> sage: sage.graphs.graph_plot.DEFAULT_PLOT_OPTIONS['edge_color']='red'
sage: g = graphs.PetersenGraph()
sage: g.plot() # red edges
sage: g.show() # does not show anything
sage: g = Graph({3:[4,5]})
sage: g.plot() # red edges
sage: g.show() # does not show anything
</pre><ol class="upperalpha" start="4"><li>This is now ok:
</li></ol><pre class="wiki"> sage: g = graphs.PetersenGraph()
sage: p = g.graphplot()
sage: p.plot(my_favorite_useless_option_I_always_used_in_my_code=45)
/Users/slabbe/Applications/sage-5.6.rc0/local/bin/sage-ipython:1: DeprecationWarning: This method takes no argument ! You may want to give it as an argument to graphplot instead.
See http://trac.sagemath.org/13891 for details.
#!/usr/bin/env python
</pre>
TicketncohenWed, 30 Jan 2013 13:11:04 GMTattachment set
https://trac.sagemath.org/ticket/13891
https://trac.sagemath.org/ticket/13891
<ul>
<li><strong>attachment</strong>
set to <em>trac_13891-fifth_pass.patch</em>
</li>
</ul>
TicketncohenWed, 30 Jan 2013 13:14:21 GMTattachment set
https://trac.sagemath.org/ticket/13891
https://trac.sagemath.org/ticket/13891
<ul>
<li><strong>attachment</strong>
set to <em>trac_13891-bugfix.patch</em>
</li>
</ul>
TicketncohenWed, 30 Jan 2013 13:23:15 GMTstatus changed
https://trac.sagemath.org/ticket/13891#comment:46
https://trac.sagemath.org/ticket/13891#comment:46
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Soooooooooooo !
</p>
<p>
I just refolded them all to check where the missing line in <code>G.show()</code> was coming from, to notice that I stupidly removed it myself in the "fourth pass". Well.
</p>
<ul><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/13891/trac_13891-fourth_pass.patch" title="Attachment 'trac_13891-fourth_pass.patch' in Ticket #13891">trac_13891-fourth_pass.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/13891/trac_13891-fourth_pass.patch" title="Download"></a> is left unchanged, even if what it does is stupid
</li><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/13891/trac_13891-fifth_pass.patch" title="Attachment 'trac_13891-fifth_pass.patch' in Ticket #13891">trac_13891-fifth_pass.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/13891/trac_13891-fifth_pass.patch" title="Download"></a> fixes it, and also applies Punarbasu's modification
</li><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/13891/trac_13891-bugfix.patch" title="Attachment 'trac_13891-bugfix.patch' in Ticket #13891">trac_13891-bugfix.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/13891/trac_13891-bugfix.patch" title="Download"></a> removes the useless option in the combinat/ directory.
</li></ul><p>
And of course <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/13891/trac_13891-all_in_one.patch" title="Attachment 'trac_13891-all_in_one.patch' in Ticket #13891">trac_13891-all_in_one.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/13891/trac_13891-all_in_one.patch" title="Download"></a> applies all that. Now, to answer Sebastian's questions :
</p>
<p>
1) Fixed in <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/13891/trac_13891-fifth_pass.patch" title="Attachment 'trac_13891-fifth_pass.patch' in Ticket #13891">trac_13891-fifth_pass.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/13891/trac_13891-fifth_pass.patch" title="Download"></a>, sorry for that, good job noticing it as no doctest checks that <code>:-P</code>
2) Fixed in <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/13891/trac_13891-bugfix.patch" title="Attachment 'trac_13891-bugfix.patch' in Ticket #13891">trac_13891-bugfix.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/13891/trac_13891-bugfix.patch" title="Download"></a>
A) Noooooo idea on earth.
B) Yep. Because basically, <code>show</code> calls <code>plot</code>.
</p>
<p>
Well, if you like it, this "small patch that defines new default options" can go <code>:-P</code>
</p>
<p>
Thanks for your help !!!
</p>
<p>
Nathann
</p>
<p>
Apply trac_13891-all_in_one.patch
</p>
TicketppurkaWed, 30 Jan 2013 13:48:50 GMT
https://trac.sagemath.org/ticket/13891#comment:47
https://trac.sagemath.org/ticket/13891#comment:47
<p>
About A - the reason is that figsize is a property that is handled by matplotlib.
It is only the show method that actually calls matplotlib.
The plot method just prepares the set of points that are going to be on the figure and that will eventually be passed on to matplotlib.
</p>
TicketslabbeWed, 30 Jan 2013 14:58:11 GMT
https://trac.sagemath.org/ticket/13891#comment:48
https://trac.sagemath.org/ticket/13891#comment:48
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13891#comment:47" title="Comment 47">ppurka</a>:
</p>
<blockquote class="citation">
<p>
About A - the reason is that figsize is a property that is handled by matplotlib.
</p>
</blockquote>
<p>
So if somebody uses <code>show_default(True)</code> which is the actual default, then <code>show</code> is automatically called (with no option) when <code>plot</code> is called, and there is no way of changing the <code>figsize</code> through the <code>plot</code> method. Right? This is the kind of thing that made plot and show mysterious to me when I started using sage. Now, I am starting to understand!
</p>
TicketslabbeWed, 30 Jan 2013 15:25:50 GMTattachment set
https://trac.sagemath.org/ticket/13891
https://trac.sagemath.org/ticket/13891
<ul>
<li><strong>attachment</strong>
set to <em>trac_13891_note-sl.patch</em>
</li>
</ul>
TicketslabbeWed, 30 Jan 2013 15:41:32 GMT
https://trac.sagemath.org/ticket/13891#comment:49
https://trac.sagemath.org/ticket/13891#comment:49
<ol><li>I just added a patch which adds the notes about what dict affect what function. Do you agree to fold it into yours?
</li></ol><ol start="2"><li>Another question (it should be my last now!!): What are the available options for <code>show</code> or for <code>DEFAULT_SHOW_OPTIONS</code> ?
</li></ol><p>
To know available options for <code>plot</code>, I may look at the keywords of the default dict. I think they are complete right?
</p>
<pre class="wiki">sage: sage.graphs.graph_plot.DEFAULT_PLOT_OPTIONS.keys()
['edge_color', 'tree_orientation', 'layout', 'vertex_labels', 'max_dist',
'partition', 'loop_size', 'heights', 'color_by_label', 'dist', 'graph_border',
'iterations', 'edge_colors', 'edge_style', 'edge_labels', 'vertex_size','talk']
</pre><p>
or at the following docstring :
</p>
<pre class="wiki">sage: g.plot? # they do not corresponds exactly with the defaut dict, but it is not too bad
</pre><p>
But, for show, it seems more difficult to know the possible options. The keywords of the default dict contain only one :
</p>
<pre class="wiki">sage: sage.graphs.graph_plot.DEFAULT_SHOW_OPTIONS.keys()
['figsize']
</pre><p>
and the docstring of <code>show</code> does not say much about those options "not used by plot" :
</p>
<pre class="wiki">sage: g = graphs.PetersenGraph()
sage: g.show?
Docstring:
Shows the (di)graph.
For syntax and lengthy documentation, see G.plot?. Any options not
used by plot will be passed on to the Graphics.show method.
Note: See the documentation of the "sage.graphs.graph_plot" module for
information on default arguments of this method.
EXAMPLES:
sage: C = graphs.CubeGraph(8)
sage: P = C.plot(vertex_labels=False, vertex_size=0, graph_border=True)
sage: P.show() # long time (3s on sage.math, 2011)
</pre><p>
Could we add a sentence in the above show docstring explaining where we can find what are the possible options for show accepted by matplotlib?
</p>
TicketncohenWed, 30 Jan 2013 18:27:09 GMTattachment set
https://trac.sagemath.org/ticket/13891
https://trac.sagemath.org/ticket/13891
<ul>
<li><strong>attachment</strong>
set to <em>trac_13891-docagain.patch</em>
</li>
</ul>
TicketncohenWed, 30 Jan 2013 18:35:29 GMTattachment set
https://trac.sagemath.org/ticket/13891
https://trac.sagemath.org/ticket/13891
<ul>
<li><strong>attachment</strong>
set to <em>trac_13891-otherfixes.patch</em>
</li>
</ul>
TicketncohenWed, 30 Jan 2013 18:36:03 GMTattachment set
https://trac.sagemath.org/ticket/13891
https://trac.sagemath.org/ticket/13891
<ul>
<li><strong>attachment</strong>
set to <em>trac_13891-all_in_one.patch</em>
</li>
</ul>
<p>
Apply only this.
</p>
TicketncohenWed, 30 Jan 2013 18:37:02 GMT
https://trac.sagemath.org/ticket/13891#comment:50
https://trac.sagemath.org/ticket/13891#comment:50
<p>
Added <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/13891/trac_13891-docagain.patch" title="Attachment 'trac_13891-docagain.patch' in Ticket #13891">trac_13891-docagain.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/13891/trac_13891-docagain.patch" title="Download"></a> and <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/13891/trac_13891-otherfixes.patch" title="Attachment 'trac_13891-otherfixes.patch' in Ticket #13891">trac_13891-otherfixes.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/13891/trac_13891-otherfixes.patch" title="Download"></a>. They're folded inside of <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/13891/trac_13891-all_in_one.patch" title="Attachment 'trac_13891-all_in_one.patch' in Ticket #13891">trac_13891-all_in_one.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/13891/trac_13891-all_in_one.patch" title="Download"></a>.
</p>
<p>
Nathann
</p>
<p>
Apply trac_13891-all_in_one.patch
</p>
TicketppurkaThu, 31 Jan 2013 04:41:01 GMT
https://trac.sagemath.org/ticket/13891#comment:51
https://trac.sagemath.org/ticket/13891#comment:51
<p>
The options for show can be obtained directly from the top level function <code>show</code>. It is defined in safe.plot.graphics.show
</p>
TicketslabbeThu, 31 Jan 2013 19:24:51 GMTstatus changed
https://trac.sagemath.org/ticket/13891#comment:52
https://trac.sagemath.org/ticket/13891#comment:52
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13891#comment:43" title="Comment 43">ppurka</a>:
</p>
<blockquote class="citation">
<p>
@slabbe - if you have no other problems with this patch, then you can set it to positive review.
</p>
</blockquote>
<p>
Thanks Nathann for your quick patch fixes and Punarbasu for your quick responses to my too many questions. Like you said, the patch now makes much more than the initial goal, but I think it is a good thing.
</p>
<p>
Sébastien
</p>
TicketncohenThu, 31 Jan 2013 20:35:51 GMT
https://trac.sagemath.org/ticket/13891#comment:53
https://trac.sagemath.org/ticket/13891#comment:53
<blockquote class="citation">
<p>
Thanks Nathann for your quick patch fixes and Punarbasu for your quick responses to my too many questions. Like you said, the patch now makes much more than the initial goal, but I think it is a good thing.
</p>
</blockquote>
<p>
Wouhouuuuuuuuuuuuuuuuuuuuuu !! Thaaaaaaaaaaaaanks ! <code>:-)</code>
</p>
<p>
Nathann
</p>
TicketppurkaFri, 01 Feb 2013 02:56:42 GMT
https://trac.sagemath.org/ticket/13891#comment:54
https://trac.sagemath.org/ticket/13891#comment:54
<p>
Thanks Sebastien for pushing both of us - me to have a look at this again, and Nathann for doing all the grunt work ;)
</p>
TicketjdemeyerTue, 05 Feb 2013 08:20:28 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/13891#comment:55
https://trac.sagemath.org/ticket/13891#comment:55
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-5.7.beta3</em>
</li>
</ul>
TicketwasWed, 22 May 2013 17:02:14 GMT
https://trac.sagemath.org/ticket/13891#comment:56
https://trac.sagemath.org/ticket/13891#comment:56
<p>
This has annoyed the hell out of me about 100 times in the last month:
</p>
<pre class="wiki">g.plot(svg=True)
127
True
/usr/local/sage/sage-5.10.beta3/local/lib/python2.7/site-packages/sage/graphs/generic_graph.py:14263: DeprecationWarning: You provided svg as an argument to a function which has always silently ignored its inputs. This method may soon be updated so that the method raises an exception instead of this warning, which will break your code : to be on the safe side, update it !
See http://trac.sagemath.org/13891 for details.
return GraphPlot(graph=self, options=options)
</pre><p>
I strongly disagree with this deprecation. It is an extremely useful feature that the plotting functions hold their arguments, then pass the unused arguments on to show, just like the do (by design!) in 3d graphics, and did do (by design!) for many years in 2d graphics.
</p>
TicketncohenWed, 22 May 2013 19:26:20 GMT
https://trac.sagemath.org/ticket/13891#comment:57
https://trac.sagemath.org/ticket/13891#comment:57
<blockquote class="citation">
<p>
This has annoyed the hell out of me about 100 times in the last month:
</p>
</blockquote>
<p>
Your turn. Silently ignored arguments annoyed the hell out of me for a while too. I had no idea why nothing happened.
</p>
<blockquote class="citation">
<p>
I strongly disagree with this deprecation. It is an extremely useful feature that the plotting functions hold their arguments, then pass the unused arguments on to show, just like the do (by design!) in 3d graphics, and did do (by design!) for many years in 2d graphics.
</p>
</blockquote>
<p>
I have nothing against passing arguments from a function to another. I have something against failing silently. Now we have a list of arguments which are understood and used by show/plot, and you can use that to check that what you forward to another function will actually be used. You can also do the same everywhere else, and add these flags to the the documentation, which is far from being a rule everywhere else.
</p>
<p>
<a href="http://www.sagemath.org/doc/reference/plotting/sage/graphs/graph_plot.html">http://www.sagemath.org/doc/reference/plotting/sage/graphs/graph_plot.html</a>
</p>
<p>
Nathann
</p>
TicketslabbeThu, 23 May 2013 09:36:38 GMT
https://trac.sagemath.org/ticket/13891#comment:58
https://trac.sagemath.org/ticket/13891#comment:58
<blockquote class="citation">
<p>
I strongly disagree with this deprecation.
</p>
</blockquote>
<p>
I agree we should never deprecate it as I guess so much code are using useless arguments out there. Maybe the "<a class="missing wiki">DeprecationWarning?</a>" was not well chosen, just a "Warning" would have been ok without mentionning any future deprecation. So, what annoyed you in fact? The deprecation? or the warning that was printed 100 times?
</p>
<p>
On my side, I would be happy to learn that the input I am giving to a function is just totally ignored. Why? Because this is just normal Python I am used to (we can also think of a mispell argument written by dyslexicy) :
</p>
<pre class="wiki">sage: def f(a,b): return a+b
sage: f(4,5)
9
sage: f(4,5,color='blue')
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
<ipython-input-15-d3f7a3aa970a> in <module>()
----> 1 f(Integer(4),Integer(5),color='blue')
TypeError: f() got an unexpected keyword argument 'color'
</pre>
TicketppurkaThu, 23 May 2013 11:53:21 GMT
https://trac.sagemath.org/ticket/13891#comment:59
https://trac.sagemath.org/ticket/13891#comment:59
<p>
I agree that options should not be ignored silently - so in that sense I agree with Nathann.
</p>
<p>
On the other hand, it seems that some other plot functions silently pass on their options to the show() method. I think something similar should be done here. I have opened <a class="closed ticket" href="https://trac.sagemath.org/ticket/14632" title="defect: make generic_graph.plot() pass its options to show (closed: fixed)">#14632</a> to track this.
</p>
TicketwasThu, 23 May 2013 20:53:40 GMT
https://trac.sagemath.org/ticket/13891#comment:60
https://trac.sagemath.org/ticket/13891#comment:60
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13891#comment:59" title="Comment 59">ppurka</a>:
</p>
<blockquote class="citation">
<p>
I agree that options should not be ignored silently - so in that sense
I agree with Nathann.
</p>
</blockquote>
<p>
I also agree that options should not be completely ignored silently. I want them to be passed on to the show command, which would then complain if the options are invalid.
</p>
<blockquote class="citation">
<p>
On the other hand, it seems that some other plot functions silently pass
on their options to the show() method. I think something similar should
be done here. I have opened <a class="closed ticket" href="https://trac.sagemath.org/ticket/14632" title="defect: make generic_graph.plot() pass its options to show (closed: fixed)">#14632</a> to track this.
</p>
</blockquote>
<p>
Looking now, I see that I agree with what you propose there.
</p>
Ticket