Default parameters for Graph.plot() and Graph.show()

[nathanncohen]

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.

Here are the new lines in my init.sage file :

import sage.graphs.graph_plot
sage.graphs.graph_plot.default_show_options['figsize'] = 15

Hell Yeahhhhhhhhhhhhhhhhhhh !! :-P

Nathann

---

Apply to devel/sage

1. attachment: trac_13891-all_in_one.patch

Depends on #13862

CC: @dcoudert @nthiery @hivert @videlec

Component: graph theory

Author: Nathann Cohen

Reviewer: Punarbasu Purkayastha, Sébastien Labbé

Merged: sage-5.7.beta3

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

[ppurka]

comment:2

I think the option could be termed as SHOW_OPTIONS to keep the terminology compatible/similar with what is there in sage.plot.graphics.

Also, why did you decide to create another dictionary, when there is already graphplot_options available?

[nathanncohen]

comment:3

I think the option could be termed as SHOW_OPTIONS to keep the terminology compatible/similar with what is there in sage.plot.graphics.

Really ? O_o
Upper case variables are so ugly...

Also, why did you decide to create another dictionary, when there is already graphplot_options available?

Well, because the graphplot_options dictionary seems to associate text descriptions to keywords, while I want to associate actual values.

Btw, I will try to deal with the pictures of our Belgian evening in Singapour sooner or later :-)

Have fuuuuuuuuuuuuuuuuuunn !

Nathann

[ppurka]

comment:4

Replying to @nathanncohen:

I think the option could be termed as SHOW_OPTIONS to keep the terminology compatible/similar with what is there in sage.plot.graphics.

Really ? O_o
Upper case variables are so ugly...

Yes, they are ugly. But this acts like a global variable in the file. Also if (and that's a big if, looking at my near future) I get time, I will try to clean up some of the options in plots (#13828), so I would prefer there are less inconsistencies introduced in new patches! :)

Also, why did you decide to create another dictionary, when there is already graphplot_options available?

Well, because the graphplot_options dictionary seems to associate text descriptions to keywords, while I want to associate actual values.

Whoops! Of course! That was so silly of me.

Btw, I will try to deal with the pictures of our Belgian evening in Singapour sooner or later :-)

Great! By the way that's a nice way to mention Singapore - it is really pouring all day long here!

Have fuuuuuuuuuuuuuuuuuunn !

You too! Wish you a happy new year!

Nathann

[nathanncohen]

comment:5

Helloooooooo !!

Yes, they are ugly.

I am glad we agree on that. Patch updated, ugliness included.
God I hate upper case variables.
And I hate standards even more.

Great! By the way that's a nice way to mention Singapore - it is really pouring all day long here!

Oops sorry ! French spelling :-)

Have fuuuuuuuuuuuuuuuuuuuuuunnn !!

Nathann

[nthiery]

Replying to @nathanncohen:

import sage.graphs.graph_plot
sage.graphs.graph_plot.default_show_options['figsize'] = 15

Gosh, I love that feature!

Actually, I would love it even more if it could be set for all plots at once.

Happy new year!

[ppurka]

comment:7

Replying to @nthiery:

Actually, I would love it even more if it could be set for all plots at once.

Here's a (the?) way

sage.plot.graphics.Graphics.SHOW_OPTIONS['figsize'] = 15

[nathanncohen]

comment:8

Here's a (the?) way

sage.plot.graphics.Graphics.SHOW_OPTIONS['figsize'] = 15

>_<

Ok... Perhaps this should be written in the doc somewhere ? :-P

Nathann

[nathanncohen]

comment:9

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 :-P

Nathann

P.S. I will update it in a couple of seconds to include Punarbasu's (very sensible) remark :-P

[ppurka]

comment:10

@nathanncohen: First of call, thanks for this cleanup! Now, here are some comments:

1. there seems to be a doctest failure according to the patchbot.
2. Just a small typo here (dictionary):

+        # This dictinary only contains the options that graphplot

4. Some more typos.. s/explicitely/explicitly
5. About your major change - I will need to look at it more carefully. It seems strange to me that GraphPlot imports Graphics from sage.plot and doesn't make use of the figsize parameter from there. I don't understand why it behaves so.

[nathanncohen]

comment:11

Patch updated ! As for the bug, it's yet another combinat side-effect >_<

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.

Vincent : it seems to come from your patch #11422. How do you propose that we fix this ?

Nathann

[nathanncohen]

comment:12

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 ?

Nathann

[videlec]

comment:13

Hi Nathann,

Vincent : it seems to come from your patch #11422. How do you propose that we fix this ?

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

sage: G = ArithmeticSubgroup_Permutation(S2="(1,2)(3,4)",S3="(1,2,3)")                
sage: G.coset_graph().show()

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.

Cheers,
Vincent

[nathanncohen]

comment:14

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 :

@options()
def plot ....

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.

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.

Nathann

[nathanncohen]

comment:15

Here it is, with the solution given above. This patch is waiting for a review again :-P

Nathann

[seblabbe]

comment:16

Attachment: trac_13891.patch.gz

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

1. In the documentation of plot method, I do not understand this:

- Default parameters for this method can also be set through the 
  :class:`~sage.misc.decorators.options` mechanism. 

Can you provide an example instead? like:

  
  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() 

2. Same comment for show method. I prefer to see an example in the doc which illustrates the utilisation of this default parameters.

3. Add example in both plot and show like this one:

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()

4. dictinary still appears

The following point are just questions...

A. Somebody can explain me what is the specification differences between show and plot?

B. Why do you remove the element from the kwds? Is there some efficiency gain?

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} 

C. Should not {} be the default ? In genereal, this should be a dict. I guess None is fine.

"edge_colors"         : None, 

D. What is this?

 	69	Methods and classes 
 	70	------------------- 
 	71	.. autofunction:: _circle_embedding 
 	72	.. autofunction:: _line_embedding 

[seblabbe]

Reviewer: Punarbasu Purkayastha, Sébastien Labbé

[ppurka]

comment:18

Replying to @seblabbe:

A. Somebody can explain me what is the specification differences between show and plot?

plot returns an object of class Graphics. show actually calls matplotlib, renders the Graphics 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

(graphs.CycleGraph(3).plot() + graphs.CycleGraph(5).plot()).show()

B. Why do you remove the element from the kwds? Is there some efficiency gain?

This is fine. It is not for efficiency, but to remove the kwds which are not handled by the GraphPlot class.

C. Should not {} be the default ? In genereal, this should be a dict. I guess None is fine.

"edge_colors"         : None, 

I have no opinion on this. As long as it works, I am fine with either of None or {}. :)

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 SHOW_OPTIONS from sage/plot/graphics.py (by using the method I mentioned in comment:7) and have the same value reflected in the GraphPlot code. The last time I had checked this, it didn't work.

[seblabbe]

comment:19

Replying to @ppurka:

Replying to @seblabbe:

A. Somebody can explain me what is the specification differences between show and plot?

plot returns an object of class Graphics. show actually calls matplotlib, renders the Graphics object, and then displays the result.

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.

Thanks for your answers!

I will continue my review (on sage-5.6.rc0 the patch applies fine) and let you check the other SHOW_OPTIONS thing.

Sébastien

[ppurka]

comment:20

Replying to @seblabbe:

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.

Actually, plot never returns the rendered graphics object. What happens when you don't assign any variable to plot like

a = plot(...) # assigning the output of plot to variable a

then the _repr_() method of the Graphics class is called. This is similar to the way the print function in Sage works. The _repr_ method then implicitly calls show and your graph gets rendered and displayed.

[seblabbe]

comment:21

One more point. This does not affect the result of plot:

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()

One may say it is because I should use DEFAULT_PLOT_OPTIONS instead. But it is not the case. As the following is not affected by changing DEFAULT_PLOT_OPTIONS :

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()

Hence, I believe the documentation at the start of the file graph_plot.py should contain at least one example using DEFAULT_PLOT_OPTIONS and one example using DEFAULT_SHOW_OPTIONS. As a user I want to understand why there are two such dict and which one I should use.

[seblabbe]

comment:22

Replying to @ppurka:

The _repr_ method then implicitly calls show and your graph gets rendered and displayed.

So, I believe this is the reason of my confusion. Do we really want a string representation function like _repr_ to display a png? If I think about it, I don't like it! I prefer the string representation to tell me that the GraphicObject is made of 345 primitive graphics objects. And then, I decide whether I show it or not...

[ppurka]

comment:23

Replying to @seblabbe:

So, I believe this is the reason of my confusion. Do we really want a string representation function like _repr_ to display a png? If I think about it, I don't like it! I prefer the string representation to tell me that the GraphicObject is made of 345 primitive graphics objects. And then, I decide whether I show it or not...

You can do so too :) Just run show_default(False) and you won't get those rendered plots.

[ppurka]

comment:44

Patchbot apply trac_13891-all_in_one.patch

[seblabbe]

comment:45

1. show method of a graph doesn't do anything now. There is a line missing in the code.

2. 2 doctests are still failing (see Comment 42)

Questions.

A. Sorry to ask this question, but why is figsize not an option for plot again?

B. If I understand correctly,

• DEFAULT_SHOW_OPTIONS will affect g.show().
• DEFAULT_PLOT_OPTIONS will affect g.plot().

but

• DEFAULT_SHOW_OPTIONS will not affect g.plot().
• DEFAULT_PLOT_OPTIONS will affect g.show().

Right?

Comments.

C. This works:

    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

D. This is now ok:

   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

[nathanncohen]

Attachment: trac_13891-fifth_pass.patch.gz

[nathanncohen]

comment:46

Attachment: trac_13891-bugfix.patch.gz

Soooooooooooo !

I just refolded them all to check where the missing line in G.show() was coming from, to notice that I stupidly removed it myself in the "fourth pass". Well.

• attachment: trac_13891-fourth_pass.patch is left unchanged, even if what it does is stupid
• attachment: trac_13891-fifth_pass.patch fixes it, and also applies Punarbasu's modification
• attachment: trac_13891-bugfix.patch removes the useless option in the combinat/ directory.

And of course attachment: trac_13891-all_in_one.patch applies all that. Now, to answer Sebastian's questions :

1. Fixed in attachment: trac_13891-fifth_pass.patch, sorry for that, good job noticing it as no doctest checks that :-P
2. Fixed in attachment: trac_13891-bugfix.patch
   A) Noooooo idea on earth.
   B) Yep. Because basically, show calls plot.

Well, if you like it, this "small patch that defines new default options" can go :-P

Thanks for your help !!!

Nathann

Apply trac_13891-all_in_one.patch

[ppurka]

comment:47

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.

[seblabbe]

comment:48

Replying to @ppurka:

About A - the reason is that figsize is a property that is handled by matplotlib.

So if somebody uses show_default(True) which is the actual default, then show is automatically called (with no option) when plot is called, and there is no way of changing the figsize through the plot 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!

[seblabbe]

comment:49

Attachment: trac_13891_note-sl.patch.gz

1. I just added a patch which adds the notes about what dict affect what function. Do you agree to fold it into yours?

2. Another question (it should be my last now!!): What are the available options for show or for DEFAULT_SHOW_OPTIONS ?

To know available options for plot, I may look at the keywords of the default dict. I think they are complete right?

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']

or at the following docstring :

sage: g.plot?  # they do not corresponds exactly with the defaut dict, but it is not too bad

But, for show, it seems more difficult to know the possible options. The keywords of the default dict contain only one :

sage: sage.graphs.graph_plot.DEFAULT_SHOW_OPTIONS.keys()
['figsize']

and the docstring of show does not say much about those options "not used by plot" :

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)

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?

[nathanncohen]

Attachment: trac_13891-docagain.patch.gz

Attachment: trac_13891-otherfixes.patch.gz

[nathanncohen]

Attachment: trac_13891-all_in_one.patch.gz

Apply only this.

[nathanncohen]

comment:50

Added attachment: trac_13891-docagain.patch and attachment: trac_13891-otherfixes.patch. They're folded inside of attachment: trac_13891-all_in_one.patch.

Nathann

Apply trac_13891-all_in_one.patch

[ppurka]

comment:51

The options for show can be obtained directly from the top level function show. It is defined in safe.plot.graphics.show

[seblabbe]

comment:52

Replying to @ppurka:

@seblabbe - if you have no other problems with this patch, then you can set it to positive review.

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.

Sébastien

[nathanncohen]

comment:53

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.

Wouhouuuuuuuuuuuuuuuuuuuuuu !! Thaaaaaaaaaaaaanks ! :-)

Nathann

[ppurka]

comment:54

Thanks Sebastien for pushing both of us - me to have a look at this again, and Nathann for doing all the grunt work ;)

[jdemeyer]

Merged: sage-5.7.beta3

[williamstein]

comment:56

This has annoyed the hell out of me about 100 times in the last month:

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)

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.

[nathanncohen]

comment:57

This has annoyed the hell out of me about 100 times in the last month:

Your turn. Silently ignored arguments annoyed the hell out of me for a while too. I had no idea why nothing happened.

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.

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.

http://www.sagemath.org/doc/reference/plotting/sage/graphs/graph_plot.html

Nathann

[seblabbe]

comment:58

I strongly disagree with this deprecation.

I agree we should never deprecate it as I guess so much code are using useless arguments out there. Maybe the "DeprecationWarning" 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?

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

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'

[ppurka]

comment:59

I agree that options should not be ignored silently - so in that sense I agree with Nathann.

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 #14632 to track this.

[williamstein]

comment:60

Replying to @ppurka:

I agree that options should not be ignored silently - so in that sense
I agree with Nathann.

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.

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 #14632 to track this.

Looking now, I see that I agree with what you propose there.