Opened 7 years ago

Last modified 6 years ago

#13891 closed enhancement

Default parameters for Graph.plot() and Graph.show() — at Version 44

Reported by: ncohen Owned by: jason, ncohen, rlm
Priority: major Milestone: sage-5.7
Component: graph theory Keywords:
Cc: dcoudert, nthiery, hivert, vdelecroix Merged in:
Authors: Nathann Cohen Reviewers: Punarbasu Purkayastha, Sébastien Labbé
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #13862 Stopgaps:

Description (last modified by ppurka)

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. trac_13891-all_in_one.patch

Change History (48)

comment:1 Changed 7 years ago by ncohen

  • Status changed from new to needs_review

comment:2 follow-up: Changed 7 years ago by ppurka

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?

comment:3 in reply to: ↑ 2 ; follow-up: Changed 7 years ago by ncohen

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

comment:4 in reply to: ↑ 3 ; follow-up: Changed 7 years ago by ppurka

Replying to ncohen:

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

comment:5 in reply to: ↑ 4 Changed 7 years ago by ncohen

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

comment:6 in reply to: ↑ description ; follow-up: Changed 7 years ago by nthiery

Replying to ncohen:

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!

comment:7 in reply to: ↑ 6 ; follow-up: Changed 7 years ago by ppurka

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

comment:8 in reply to: ↑ 7 Changed 7 years ago by ncohen

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

comment:9 Changed 7 years ago by ncohen

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

comment:10 Changed 7 years ago by ppurka

@ncohen: 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
    
  3. Some more typos.. s/explicitely/explicitly
  4. 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.

comment:11 follow-up: Changed 7 years ago by ncohen

  • Cc vdelecroix added

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

comment:12 Changed 7 years ago by ncohen

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

comment:13 in reply to: ↑ 11 Changed 7 years ago by vdelecroix

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

Last edited 7 years ago by vdelecroix (previous) (diff)

comment:14 Changed 7 years ago by ncohen

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

comment:15 Changed 7 years ago by ncohen

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

Nathann

Changed 7 years ago by ncohen

comment:16 Changed 7 years ago by slabbe

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() 
  1. Same comment for show method. I prefer to see an example in the doc which illustrates the utilisation of this default parameters.
  1. 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()

  1. dictinary still appears

The following point are just questions...

  1. Somebody can explain me what is the specification differences between show and plot?
  1. 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} 
  1. Should not {} be the default ? In genereal, this should be a dict. I guess None is fine.
"edge_colors"         : None, 
  1. What is this?
 	69	Methods and classes 
 	70	------------------- 
 	71	.. autofunction:: _circle_embedding 
 	72	.. autofunction:: _line_embedding 
Last edited 7 years ago by slabbe (previous) (diff)

comment:17 Changed 7 years ago by slabbe

  • Reviewers set to Punarbasu Purkayastha, Sébastien Labbé

comment:18 follow-up: Changed 7 years ago by ppurka

Replying to slabbe:

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

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

comment:19 in reply to: ↑ 18 ; follow-up: Changed 7 years ago by slabbe

Replying to ppurka:

Replying to slabbe:

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

comment:20 in reply to: ↑ 19 ; follow-up: Changed 7 years ago by ppurka

Replying to slabbe:

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.

comment:21 Changed 7 years ago by slabbe

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.

comment:22 in reply to: ↑ 20 ; follow-up: Changed 7 years ago by slabbe

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

comment:23 in reply to: ↑ 22 ; follow-up: Changed 7 years ago by ppurka

Replying to slabbe:

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.

comment:24 Changed 7 years ago by ncohen

Hellooooooooo Sebastien !

I can now answer your many comments, and write down what we said orally across the table :-)

1/2) I fixed this doc, but only where it is related to what this ticket implements, and not related to the "@options" decorator. This has to be documented where the decorator is defined, not in these files.

3) There is a link from the documentation of show toward graph_plot

4) Fixed

D) Because I want those two methods to appear in the doc, even if they begin with a _

What this new patch does :

1) It changes the documentation of GraphPlot? so that the list of admissible keywords appear only once, and in the documentation. 2) The GraphPlot?.plot() method accepts any options, and ignores them. I remove the **kwds 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

Tell me what you think of it !

Nathann

Changed 7 years ago by ncohen

comment:25 in reply to: ↑ 23 Changed 7 years ago by slabbe

Replying to ppurka:

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

Great!!! Thanks a lot for this trick!

comment:26 Changed 7 years ago by slabbe

Great for the second patch. I like the improvements in the documentation. However, here are some issues:

  1. The following used to work, but is broken by the actual patches. A deprecation warning is needed :
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'
  1. (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 'scaling_term' 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:
 from sage.misc.superseded import deprecation 
-deprecation(13891, (str(opt)+" is not a registered argument for"+ 
-                            "GraphPlot ! Please tell us that if it "+ 
-                            "should be by writing to sage-devel !")) 
+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."))
  1. (Remark) BTW, the + are not necessary above. For instance:
sage: ('first  line'
....:      'second line\n'
....:    'third line')
'first  linesecond line\nthird line'
  1. There are some errors reported by the buildbot:
	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

comment:27 Changed 7 years ago by slabbe

  • Status changed from needs_review to needs_work

comment:28 Changed 7 years ago by ncohen

  • Status changed from needs_work to needs_review

Updaaaaaaaaaated ! :-P

Nathann

comment:29 Changed 7 years ago by ppurka

  • Description modified (diff)

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

  1. There is a doctest error
    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\'``')]
    
  2. I think the following should mention something like "You may want to give it as an argument to graphplot() instead" (i.e. the GraphPlot should be all lower case)
    865	            deprecation(13891, "This method takes no argument ! You may want " 
    866	                               "to give it to GraphPlot instead.")
    

comment:30 Changed 7 years ago by ncohen

Patch updated :-)

Nathann

Changed 7 years ago by ncohen

comment:31 Changed 7 years ago by ppurka

I saw a few more documentation problems that are a leftover from your earlier patches. Noticing this only now - sorry :-/ The following should be GenericGraph.graphplot since plot doesn't take any options now.

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 

Shouldn't this be changed now? Now, graph.plot() doesn't take anything.

    def graphplot(self, **options):
        """
        Returns a GraphPlot object.

        EXAMPLES:

        Creating a graphplot object uses the same options as graph.plot()::

Maybe write it like this?

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

comment:32 Changed 7 years ago by ncohen

Hellooooooooooo !!!

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 ?

Nathann

comment:33 follow-up: Changed 7 years ago by slabbe

graphplot = deprecated_function_alias(13891, plot)

or just

graphplot = plot
Last edited 7 years ago by slabbe (previous) (diff)

comment:34 Changed 7 years ago by ncohen

Couldn't we just remove useless stuff and stop spending a lifetime deprecating it ? ...-_-

Nathann

comment:35 follow-up: Changed 7 years ago by slabbe

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

We should try to avoid this with Sage.

comment:36 in reply to: ↑ 33 ; follow-up: Changed 7 years ago by ppurka

Replying to slabbe:

graphplot = plot

I am actually in favor of this (actually plot = graphplot). Just make this alias so that current stuff doesn't break. Moreover, it will still remain consistent with the other commands and with the plot command from sage.plot.

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 plot = graphplot.

Last edited 7 years ago by ppurka (previous) (diff)

comment:37 in reply to: ↑ 35 Changed 7 years ago by ncohen

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

We should try to avoid this with Sage.

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.

I'm updating the patch with a deprecation warning.

Nathann

comment:38 in reply to: ↑ 36 Changed 7 years ago by ncohen

I am actually in favor of this (actually plot = graphplot).

Why would we keep graphplot and throw plot away ? plot seems much more natural than graphplot...

Just make this alias so that current stuff doesn't break.

I will just have to change the calls to graphplot to plot.

Tell me if there is anything wrong with that. If you think that it is fine I will begin fixing the doctests.

Moreover, it will still remain consistent with the other commands and with the plot command from sage.plot.

What do you mean ?

If you remove graphplot, you should revert all the warnings and deprecation that was introduced in plot.

Why ? plot does not call graphplot !

As of now, I would say, keep both and make plot = graphplot.

Whyyyyyyyyyyyyyyyyyyy ? Why should we keep two functions that do the same thing ? O_o

Nathann

comment:39 follow-up: Changed 7 years ago by ncohen

Hey, I just noticed that .graphplot and .plot just do not do the same thing.

graphplot :

return GraphPlot(graph=self, options=options)

plot :

return GraphPlot(graph=self, options=options).plot()

So it does not really make sense to deprecate one by making it an alias to the other one !

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.

Nathann

Changed 7 years ago by ncohen

comment:40 follow-up: Changed 7 years ago by ncohen

(I just updated the patch so that graphplot stays as it was, along with plot. The references to GraphPlot.plot have been replaced by GenericGraph.plot, which takes all kind of arguments.)

Nathann

comment:41 in reply to: ↑ 39 Changed 7 years ago by slabbe

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 :

- return GraphPlot(graph=self, options=options).plot()
+ return self.graphplot(options=options).plot()
Last edited 7 years ago by slabbe (previous) (diff)

comment:42 Changed 7 years ago by slabbe

There are still two failling tests in the recent buildbot:

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

comment:43 in reply to: ↑ 40 Changed 7 years ago by ppurka

Replying to ncohen:

(I just updated the patch so that graphplot stays as it was, along with plot. The references to GraphPlot.plot have been replaced by GenericGraph.plot, which takes all kind of arguments.)

Nathann

Thanks. This is a huge mess.

I understand why the plot method is needed. If you look at the code for plot(f) (the generic plot function) then, it returns the output f.plot() method if it is present. I think the current patch is OK. I also think slabbe is right in comment:41.

I have combined all the patches and made the 2-line change to GenericGraph.plot.

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

comment:44 Changed 7 years ago by ppurka

  • Description modified (diff)

Patchbot apply trac_13891-all_in_one.patch

Note: See TracTickets for help on using tickets.