#19539 closed defect (fixed)

make Graphics.plot refuses argument

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-6.10
Component: graphics Keywords:
Cc: mhs, kcrisman Merged in:
Authors: Vincent Delecroix Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: a322663 (Commits) Commit: a322663068ebaa3473dceece77a5abab81689250
Dependencies: Stopgaps:

Description

Currently Graphics.plot ignore all input arguments which make the following very confusing

sage: S = circle((0,0), 1)
sage: T = S.plot(aspect_ratio=2)
sage: T.aspect_ratio()
1.0

We simply disallow arguments in order to have

sage: S.plot(aspect_ratio=1)
Traceback (most recent call last):
...
TypeError: plot() got an unexpected keyword argument 'aspect_ratio'

Change History (13)

comment:1 Changed 23 months ago by vdelecroix

  • Branch set to u/vdelecroix/19539
  • Commit set to e3741280d9f8ece94c3f6aa0b333d9274e17cdfb
  • Status changed from new to needs_review

New commits:

e374128Trac 19539: remove args and kwds in Graphics.plot

comment:2 follow-up: Changed 23 months ago by ncohen

Just adding a comment to say that I admire your courage.

comment:3 Changed 23 months ago by git

  • Commit changed from e3741280d9f8ece94c3f6aa0b333d9274e17cdfb to 03dbb2eefc569424de29f6d36d875ba427eb7b2c

Branch pushed to git repo; I updated commit sha1. New commits:

03dbb2eTrac 19539: fix doctests

comment:4 in reply to: ↑ 2 ; follow-up: Changed 23 months ago by vdelecroix

Replying to ncohen:

Just adding a comment to say that I admire your courage.

What about a courageous review?

comment:5 in reply to: ↑ 4 Changed 23 months ago by ncohen

  • Status changed from needs_review to needs_info

What about a courageous review?

Let's try:

-    if hasattr(funcs, 'plot'):
+    if isinstance(funcs, Graphics):
+        G = funcs
+    elif hasattr(funcs, 'plot'):

Why would you ignore (without warning) the contents of *args and **kwds when 'funcs' is a Graphics object? It seems as bad as what you try to fix in this branch.

Nathann

comment:6 follow-up: Changed 23 months ago by vdelecroix

More or less. Graphics is precisely the class that implemented

def plot(self, *args, **kwds):
    return self

And nobody inherits from it.

I did it because some code is calling plot(x)... you are right that it would be better to fix it instead.

What do you think that the following should do

sage: C = circle((0,0), 1)
sage: plot(C, aspect_ratio=2)

It used to return C unchanged and it still does. What my code is modifying is

sage: C.plot(aspect_ratio=2)
... -> error

comment:7 in reply to: ↑ 6 Changed 23 months ago by ncohen

Hello,

What do you think that the following should do

sage: C = circle((0,0), 1)
sage: plot(C, aspect_ratio=2)

It used to return C unchanged and it still does. What my code is modifying is

If it ignores the 'aspect_ratio=2' argument then it should raise an exception instead, exactly as you do it in this branch for C.plot(whatever=3)?

Nathann

comment:8 Changed 23 months ago by git

  • Commit changed from 03dbb2eefc569424de29f6d36d875ba427eb7b2c to 82f3805f492bb7e9688043ae002315caef54dc83

Branch pushed to git repo; I updated commit sha1. New commits:

41dcc53Trac 19539: revert the change to the function plot
82f3805Trac 19539: fix ode_solver.plot_solutions

comment:9 Changed 23 months ago by vdelecroix

done!

comment:10 Changed 23 months ago by git

  • Commit changed from 82f3805f492bb7e9688043ae002315caef54dc83 to a322663068ebaa3473dceece77a5abab81689250

Branch pushed to git repo; I updated commit sha1. New commits:

a322663Trac 19539: fix more doctests

comment:11 Changed 23 months ago by ncohen

  • Reviewers set to Nathann Cohen
  • Status changed from needs_info to positive_review

Looks good. Thank you for this branch,

Nathann

comment:12 Changed 23 months ago by tscrim

  • Cc kcrisman added

comment:13 Changed 23 months ago by vbraun

  • Branch changed from u/vdelecroix/19539 to a322663068ebaa3473dceece77a5abab81689250
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.