Opened 2 years ago
Closed 2 years ago
#19539 closed defect (fixed)
make Graphics.plot refuses argument
Reported by:  vdelecroix  Owned by:  

Priority:  major  Milestone:  sage6.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 2 years ago by
 Branch set to u/vdelecroix/19539
 Commit set to e3741280d9f8ece94c3f6aa0b333d9274e17cdfb
 Status changed from new to needs_review
comment:2 followup: ↓ 4 Changed 2 years ago by
Just adding a comment to say that I admire your courage.
comment:3 Changed 2 years ago by
 Commit changed from e3741280d9f8ece94c3f6aa0b333d9274e17cdfb to 03dbb2eefc569424de29f6d36d875ba427eb7b2c
Branch pushed to git repo; I updated commit sha1. New commits:
03dbb2e  Trac 19539: fix doctests

comment:4 in reply to: ↑ 2 ; followup: ↓ 5 Changed 2 years ago by
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 2 years ago by
 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 followup: ↓ 7 Changed 2 years ago by
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 2 years ago by
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 2 years ago by
 Commit changed from 03dbb2eefc569424de29f6d36d875ba427eb7b2c to 82f3805f492bb7e9688043ae002315caef54dc83
comment:9 Changed 2 years ago by
done!
comment:10 Changed 2 years ago by
 Commit changed from 82f3805f492bb7e9688043ae002315caef54dc83 to a322663068ebaa3473dceece77a5abab81689250
Branch pushed to git repo; I updated commit sha1. New commits:
a322663  Trac 19539: fix more doctests

comment:11 Changed 2 years ago by
 Reviewers set to Nathann Cohen
 Status changed from needs_info to positive_review
Looks good. Thank you for this branch,
Nathann
comment:12 Changed 2 years ago by
 Cc kcrisman added
comment:13 Changed 2 years ago by
 Branch changed from u/vdelecroix/19539 to a322663068ebaa3473dceece77a5abab81689250
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Trac 19539: remove args and kwds in Graphics.plot