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: sage-7.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:

Status badges

Description (last modified by olazo)

Both

implicit_plot(x^2 +  y^2-1,(x,-1,1),(y,-1,1),aspect_ratio=1,color='red')

and

implicit_plot(x^2 +  y^2-1,(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 olazo

  • Description modified (diff)

comment:2 Changed 12 years ago by olazo

  • Keywords implicit_plot added
  • Milestone set to sage-4.5.2

comment:3 Changed 12 years ago by jason

Solved by #8529.

comment:4 Changed 11 years ago by kcrisman

  • 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 kcrisman

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 jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:7 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:8 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:9 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:10 Changed 6 years ago by paulmasson

  • Branch set to u/paulmasson/9654

comment:11 Changed 6 years ago by paulmasson

  • Authors set to Paul Masson
  • Cc kcrisman added
  • Commit set to ea98638e7b5d15b9f5b32f50144d576b339c8145
  • Milestone changed from sage-6.4 to sage-7.3
  • Status changed from new to needs_review

This appears to be a simple fix for this issue. Do I need to add anything more than the doctest?


New commits:

842fc8cAdd rgbcolor option to implict_plot
ea98638Add doctest for fix

comment:12 Changed 6 years ago by tscrim

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 follow-up: Changed 6 years ago by paulmasson

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 ; follow-up: Changed 6 years ago by tscrim

  • 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:

0129681Add ValueError and doctest

comment:15 in reply to: ↑ 14 ; follow-up: Changed 6 years ago by 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 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 tscrim

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 both color and rgbcolor. 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 git

  • Commit changed from 01296816efffbc15c9ba8613f2c48f1aa3295d90 to 5764c22a5859baea5c1825a2cbbb2837775b0db2

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

5764c22Additional doctest

comment:18 Changed 6 years ago by paulmasson

Done.

comment:19 Changed 6 years ago by tscrim

  • Status changed from needs_review to positive_review

Thanks.

comment:20 Changed 6 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:21 Changed 6 years ago by git

  • Commit changed from 5764c22a5859baea5c1825a2cbbb2837775b0db2 to 667d19c5fe3c2221b61cf684fd37c617807c8f4f

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

667d19cRemove white space

comment:22 Changed 6 years ago by git

  • Commit changed from 667d19c5fe3c2221b61cf684fd37c617807c8f4f to eefad2541be8b80919548da81b6b61c959a7667f

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

ade94e7Add rgbcolor option to implict_plot
eefad25Fix merge conflict

comment:23 Changed 6 years ago by paulmasson

  • 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 vbraun

  • 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 vbraun

  • Branch changed from u/paulmasson/9654 to eefad2541be8b80919548da81b6b61c959a7667f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.