make Graphics.plot refuses argument

[videlec]

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'

CC: @sagetrac-mhs @kcrisman

Component: graphics

Author: Vincent Delecroix

Branch/Commit: a322663

Reviewer: Nathann Cohen

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

[videlec]

Branch: u/vdelecroix/19539

[videlec]

New commits:

e374128

Trac 19539: remove args and kwds in Graphics.plot

[videlec]

Commit: e374128

[nathanncohen]

comment:2

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

[sagetrac-git]

Changed commit from e374128 to 03dbb2e

[sagetrac-git]

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

03dbb2e

Trac 19539: fix doctests

[videlec]

comment:4

Replying to @nathanncohen:

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

What about a courageous review?

[nathanncohen]

comment:5

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

[videlec]

comment:6

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

[nathanncohen]

comment:7

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

[sagetrac-git]

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

41dcc53	Trac 19539: revert the change to the function plot
82f3805	Trac 19539: fix ode_solver.plot_solutions

[sagetrac-git]

Changed commit from 03dbb2e to 82f3805

[videlec]

comment:9

done!

[sagetrac-git]

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

a322663

Trac 19539: fix more doctests

[sagetrac-git]

Changed commit from 82f3805 to a322663

[nathanncohen]

comment:11

Looks good. Thank you for this branch,

Nathann

[nathanncohen]

Reviewer: Nathann Cohen

[vbraun]

Changed branch from u/vdelecroix/19539 to a322663