Opened 6 years ago

Closed 6 years ago

#14632 closed defect (fixed)

make generic_graph.plot() pass its options to show

Reported by: ppurka Owned by: jason, ncohen, rlm
Priority: major Milestone: sage-5.10
Component: graph theory Keywords:
Cc: ncohen Merged in: sage-5.10.rc0
Authors: Punarbasu Purkayastha Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by ppurka)

Essentially what the title says. If it doesn't support the option, then there are two ways it can be handled:

  1. it will give an error from show just like the other plot functions do.
  2. it will check the dict from sage.plot.graphics.Graphics.SHOW_DEFAULT and raise an error from the generic_graph.plot() or generic_graph.graphplot() itself.

Apply to devel/sage: trac_14632-generic_graph_plot.patch

Attachments (1)

trac_14632-generic_graph_plot.patch (5.3 KB) - added by ppurka 6 years ago.
Apply to devel/sage (Update with an additional one colon)

Download all attachments as: .zip

Change History (17)

comment:1 Changed 6 years ago by was

See also #13891, where some of the behavior there should be reverted a little.

comment:2 Changed 6 years ago by ppurka

  • Cc ncohen added
  • Description modified (diff)
  • Status changed from new to needs_review

Added a patch. Please check that none of your graph plots are broken :)

comment:3 Changed 6 years ago by ncohen

  • Status changed from needs_review to needs_info

Hmmmm... What can we do about that ?

sage: graphs.PetersenGraph().plot(aewarehaerhaerherh=8)

This is the line that used to return a warning

sage: graphs.PetersenGraph().plot(aewaerh=8)
/home/ncohen/.Sage/local/lib/python2.7/site-packages/sage/graphs/generic_graph.py:14263: DeprecationWarning: You provided aewaerh 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)

Could we check that all arguments given as input are actually used somewhere ?

Nathann

comment:4 Changed 6 years ago by ppurka

Added a check for incorrect arguments. I check against graphplot_options and the show options that are already extracted.

comment:5 Changed 6 years ago by ppurka

  • Status changed from needs_info to needs_review

comment:6 Changed 6 years ago by ncohen

  • Authors set to Punarbasu Purkayastha
  • Reviewers set to Nathann Cohen
  • Status changed from needs_review to positive_review

Coooooooooooooooool ! Thank you very much :-)

Nathann

comment:7 Changed 6 years ago by ppurka

That was fast :-O Thanks!

comment:8 follow-up: Changed 6 years ago by ncohen

Come on thepatch is 5kb long. Let's not bury those for months :-P

Nathann

Changed 6 years ago by ppurka

Apply to devel/sage (Update with an additional one colon)

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

Replying to ncohen:

Come on thepatch is 5kb long. Let's not bury those for months :-P

Nathann

Thanks. You should learn some of the graphics code too! Then you can review my patches ;-). Anyway, I updated the patch with a one character change that I didn't notice for days.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 6 years ago by ncohen

Thanks. You should learn some of the graphics code too! Then you can review my patches ;-).

I'am scaaared of graphics T_T

Anyway, I updated the patch with a one character change that I didn't notice for days.

Can we submit GIT patches already ? O_o

Nathann

comment:11 in reply to: ↑ 10 Changed 6 years ago by ppurka

Replying to ncohen:

Can we submit GIT patches already ? O_o

Nathann

unfortunately, no. That's just my repo to keep track of my patches. I need to apply a bunch of them every time I need to do any work.

comment:12 follow-up: Changed 6 years ago by ncohen

Oh. Do you have many patches waiting for a review ?

Nathann

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

Replying to ncohen:

Oh. Do you have many patches waiting for a review ?

Nathann

Yeah. Many of them. :-(

comment:14 in reply to: ↑ 13 Changed 6 years ago by ncohen

Yeah. Many of them. :-(

I'll regret that...

--> give me the numbers please :-P

Nathann_who_secretly_(actually_not)_hates_graphics

comment:15 Changed 6 years ago by ppurka

Well, some of them are trivial but some are not so trivial :)

You can get the numbers from the file names here.

As for the patches themselves, I would say that

  1. You can ignore the first six patches because the first two were being reviewed, while the next four will probably never get in.
  2. Trivial patch: #13690
  3. For improving consistency the tickets are #14229, #13834, #13660
  4. For my own work, I always need to apply #13422, #13528, #14580, #12798 (which is slow during doctesting on Solaris and has no future of getting in), and the type1-fonts (which I haven't made into a proper patch),
  5. New features: #13246, #14580, #14112, #13576

(I just realized that two of them should have been set to needs review!)

comment:16 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.10.rc0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.