Opened 12 years ago
Closed 6 years ago
#9654 closed enhancement (fixed)
implicit_plot does not accept rgbcolor as keyword
Reported by:  olazo  Owned by:  olazo 

Priority:  minor  Milestone:  sage7.3 
Component:  graphics  Keywords:  implicit_plot 
Cc:  kcrisman  Merged in:  
Authors:  Paul Masson  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  eefad25 (Commits, GitHub, GitLab)  Commit:  eefad2541be8b80919548da81b6b61c959a7667f 
Dependencies:  Stopgaps: 
Description (last modified by )
Both
implicit_plot(x^2 + y^21,(x,1,1),(y,1,1),aspect_ratio=1,color='red')
and
implicit_plot(x^2 + y^21,(x,1,1),(y,1,1),aspect_ratio=1,color='red')
do not produce a red circle as would be expected. matplotlib's cmap options don't get it quite good.
Change History (25)
comment:1 Changed 12 years ago by
 Description modified (diff)
comment:2 Changed 12 years ago by
 Keywords implicit_plot added
 Milestone set to sage4.5.2
comment:3 Changed 12 years ago by
comment:4 Changed 11 years ago by
 Summary changed from implicit_plot does not accept color or rgbcolor as keywords to implicit_plot does not accept rgbcolor as keyword
Not quite. color
is ok, apparently not rgbcolor
.
implicit_plot(x^2+y^2 == 2, (x,3,3), (y,3,3), rgbcolor=(0,1,0)) verbose 0 (138: primitive.py, options) WARNING: Ignoring option 'rgbcolor'=(0, 1, 0) verbose 0 (138: primitive.py, options) The allowed options for ContourPlot defined by a 150 x 150 data grid are: cmap the name of a predefined colormap, a list of colors, or an instance of a matplotlib Colormap. Type: import matplotlib.cm; matplotlib.cm.datad.keys() for available colormap names. colorbar Include a colorbar indicating the levels colorbar_optionsa dictionary of options for colorbars contours Either an integer specifying the number of contour levels, or a sequence of numbers giving the actual contours to use. fill Fill contours or not label_options a dictionary of options for the labels labels show line labels or not legend_label The label for this item in the legend. linestyles the style of the lines to be plotted linewidths the width of the lines to be plotted plot_points How many points to use for plotting precision zorder The layer level in which to draw
comment:5 Changed 11 years ago by
The right way to do this is to use get_cmap
, but it's tricky to avoid some kind of weird circularity.
comment:6 Changed 9 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:7 Changed 8 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:8 Changed 8 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:9 Changed 8 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:10 Changed 6 years ago by
 Branch set to u/paulmasson/9654
comment:11 Changed 6 years ago by
 Cc kcrisman added
 Commit set to ea98638e7b5d15b9f5b32f50144d576b339c8145
 Milestone changed from sage6.4 to sage7.3
 Status changed from new to needs_review
comment:12 Changed 6 years ago by
You should raise an error if both color
and rgbcolor
are given akin to
sage: x,y = var('x,y') sage: plot(x^2  2, rgbcolor=(0,1,0), color='red')  RuntimeError Traceback (most recent call last) ... RuntimeError: Error in line(): option 'color' not valid.
Although I think RuntimeError
is not the correct error, nor should plot
go through so much to error out either. However that is a separate issue. The correct error is a ValueError
in this situation.
comment:13 followup: ↓ 14 Changed 6 years ago by
I've updated the branch to raise a ValueError
for conflicting input, as well as added a doctest.
The error in plot.py
appears to arise from this line:
@rename_keyword(color='rgbcolor')
Once we agree on how to handle the two arguments here, I can remove that line and update the code accordingly.
comment:14 in reply to: ↑ 13 ; followup: ↓ 15 Changed 6 years ago by
 Commit changed from ea98638e7b5d15b9f5b32f50144d576b339c8145 to 01296816efffbc15c9ba8613f2c48f1aa3295d90
 Reviewers set to Travis Scrimshaw
Replying to paulmasson:
I've updated the branch to raise a
ValueError
for conflicting input, as well as added a doctest.
Thanks. Looks good.
The error in
plot.py
appears to arise from this line:@rename_keyword(color='rgbcolor')Once we agree on how to handle the two arguments here, I can remove that line and update the code accordingly.
Which error where? I don't see any doctest failures.
New commits:
0129681  Add ValueError and doctest

comment:15 in reply to: ↑ 14 ; followup: ↓ 16 Changed 6 years ago by
Replying to tscrim:
Which error where? I don't see any doctest failures.
I was unclear: I meant the RunTime
error arising from specifying both color
and rgbcolor
. Presumably it arises from trying to rename a keyword argument with a name that already exists.
comment:16 in reply to: ↑ 15 Changed 6 years ago by
Replying to paulmasson:
Replying to tscrim:
Which error where? I don't see any doctest failures.
I was unclear: I meant the
RunTime
error arising from specifying bothcolor
andrgbcolor
. Presumably it arises from trying to rename a keyword argument with a name that already exists.
Ah, the one coming from using plot
. Yes, the failure is probably due to that. However, that is something for a separate ticket. If you could add a doctest checking that both inputs is invalid, then I will be happy to set a positive review.
comment:17 Changed 6 years ago by
 Commit changed from 01296816efffbc15c9ba8613f2c48f1aa3295d90 to 5764c22a5859baea5c1825a2cbbb2837775b0db2
Branch pushed to git repo; I updated commit sha1. New commits:
5764c22  Additional doctest

comment:18 Changed 6 years ago by
Done.
comment:21 Changed 6 years ago by
 Commit changed from 5764c22a5859baea5c1825a2cbbb2837775b0db2 to 667d19c5fe3c2221b61cf684fd37c617807c8f4f
Branch pushed to git repo; I updated commit sha1. New commits:
667d19c  Remove white space

comment:22 Changed 6 years ago by
 Commit changed from 667d19c5fe3c2221b61cf684fd37c617807c8f4f to eefad2541be8b80919548da81b6b61c959a7667f
comment:23 Changed 6 years ago by
 Status changed from needs_work to needs_review
Fixed merge conflict. Doctests all pass.
What is the protocol for this situation? Do I reset the positive review or wait for someone else? Thanks.
comment:24 Changed 6 years ago by
 Status changed from needs_review to positive_review
Just set it back to positive review if its just a straightforward merge fix
comment:25 Changed 6 years ago by
 Branch changed from u/paulmasson/9654 to eefad2541be8b80919548da81b6b61c959a7667f
 Resolution set to fixed
 Status changed from positive_review to closed
Solved by #8529.