Opened 5 years ago
Closed 5 years ago
#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, GitHub, GitLab) | 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 5 years ago by
- Branch set to u/vdelecroix/19539
- Commit set to e3741280d9f8ece94c3f6aa0b333d9274e17cdfb
- Status changed from new to needs_review
comment:2 follow-up: ↓ 4 Changed 5 years ago by
Just adding a comment to say that I admire your courage.
comment:3 Changed 5 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 ; follow-up: ↓ 5 Changed 5 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 5 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 follow-up: ↓ 7 Changed 5 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 5 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 5 years ago by
- Commit changed from 03dbb2eefc569424de29f6d36d875ba427eb7b2c to 82f3805f492bb7e9688043ae002315caef54dc83
comment:9 Changed 5 years ago by
done!
comment:10 Changed 5 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 5 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 5 years ago by
- Cc kcrisman added
comment:13 Changed 5 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