Opened 7 years ago
Closed 5 years ago
#16325 closed defect (fixed)
implicit_plot does not handle color parameter properly
Reported by:  ppurka  Owned by:  

Priority:  major  Milestone:  sage7.4 
Component:  graphics  Keywords:  
Cc:  kcrisman, paulmasson  Merged in:  
Authors:  Punarbasu Purkayastha  Reviewers:  KarlDieter Crisman, Travis Scrimshaw, Paul Masson 
Report Upstream:  N/A  Work issues:  
Branch:  daf974e (Commits, GitHub, GitLab)  Commit:  daf974e16c14dd934407f9701cdbcd9ee1544b77 
Dependencies:  Stopgaps: 
Description (last modified by )
There is no way to handle the color parameter when the fill
option is provided. implicit_plot
should also accept a fillcolor
parameter, just like plot
does, and pass fillcolor
and color
on to region_plot
as incol
and bordercol
, respectively.
Change History (27)
comment:1 Changed 7 years ago by
 Description modified (diff)
comment:2 Changed 7 years ago by
 Branch set to u/ppurka/ticket/16325
 Created changed from 05/11/14 08:48:12 to 05/11/14 08:48:12
 Modified changed from 05/11/14 09:20:14 to 05/11/14 09:20:14
comment:3 Changed 7 years ago by
 Status changed from new to needs_review
comment:4 Changed 7 years ago by
 Cc kcrisman added
 Commit set to 2ecec6b91cce92fc326ed83e1bcb051eb2e4d203
comment:5 Changed 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:6 Changed 7 years ago by
I'm getting an error, probably because this wasn't added to the list of options.
Failed example: implicit_plot(f, (3, 3), (3, 3), fill=True, fillcolor='red') Expected nothing Got: verbose 0 (154: primitive.py, options) WARNING: Ignoring option 'fillcolor'=red verbose 0 (154: 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. <snip> **********************************************************************
Yeah, this is just not working either when I try to change that piece. Is there some prereq I'm missing?
comment:7 Changed 7 years ago by
 Reviewers set to KarlDieter Crisman
 Status changed from needs_review to needs_work
comment:8 followup: ↓ 10 Changed 7 years ago by
I don't know why you are getting such errors. Things are working fine for me.
~/Installations/sagegit» sage tp long src/sage/plot too few successful tests, not using stored timings Running doctests with ID 20140816181309745f1adf. Sorting sources by runtime so that slower doctests are run first.... Doctesting 51 files using 2 threads. sage t long src/sage/plot/graphics.py [411 tests, 195.73 s] sage t long src/sage/plot/plot.py [389 tests, 230.06 s] sage t long src/sage/plot/line.py [79 tests, 38.26 s] sage t long src/sage/plot/contour_plot.py [125 tests, 91.69 s] sage t long src/sage/plot/plot3d/implicit_plot3d.py [57 tests, 37.78 s] sage t long src/sage/plot/plot3d/base.pyx [269 tests, 30.36 s] sage t long src/sage/plot/plot3d/plot3d.py [227 tests, 28.19 s] sage t long src/sage/plot/matrix_plot.py [62 tests, 27.53 s] sage t long src/sage/plot/point.py [81 tests, 23.88 s] sage t long src/sage/plot/complex_plot.pyx [31 tests, 23.32 s] sage t long src/sage/plot/polygon.py [71 tests, 18.16 s] sage t long src/sage/plot/animate.py [99 tests, 20.08 s] sage t long src/sage/plot/circle.py [54 tests, 17.22 s] sage t long src/sage/plot/arrow.py [66 tests, 17.25 s] sage t long src/sage/plot/plot3d/plot_field3d.py [8 tests, 13.80 s] sage t long src/sage/plot/plot3d/parametric_plot3d.py [196 tests, 15.08 s] sage t long src/sage/plot/plot_field.py [53 tests, 15.75 s] sage t long src/sage/plot/plot3d/tachyon.py [312 tests, 14.86 s] sage t long src/sage/plot/density_plot.py [35 tests, 14.73 s] sage t long src/sage/plot/plot3d/shapes2.py [109 tests, 11.13 s] sage t long src/sage/plot/text.py [42 tests, 9.88 s] sage t long src/sage/plot/bezier_path.py [37 tests, 8.35 s] sage t long src/sage/plot/misc.py [42 tests, 9.13 s] sage t long src/sage/plot/ellipse.py [35 tests, 8.84 s] sage t long src/sage/plot/arc.py [39 tests, 8.62 s] sage t long src/sage/plot/disk.py [55 tests, 8.50 s] sage t long src/sage/plot/plot3d/parametric_surface.pyx [85 tests, 8.03 s] sage t long src/sage/plot/colors.py [248 tests, 5.38 s] sage t long src/sage/plot/bar_chart.py [22 tests, 6.34 s] sage t long src/sage/plot/plot3d/revolution_plot3d.py [20 tests, 6.09 s] sage t long src/sage/plot/plot3d/shapes.pyx [159 tests, 6.37 s] sage t long src/sage/plot/plot3d/transform.pyx [25 tests, 4.83 s] sage t long src/sage/plot/scatter_plot.py [19 tests, 5.11 s] sage t long src/sage/plot/plot3d/list_plot3d.py [37 tests, 3.73 s] sage t long src/sage/plot/hyperbolic_arc.py [8 tests, 4.27 s] sage t long src/sage/plot/plot3d/implicit_surface.pyx [93 tests, 2.65 s] sage t long src/sage/plot/step.py [4 tests, 3.44 s] sage t long src/sage/plot/plot3d/platonic.py [41 tests, 2.79 s] sage t long src/sage/plot/primitive.py [43 tests, 2.90 s] sage t long src/sage/plot/plot3d/tri_plot.py [70 tests, 0.98 s] sage t long src/sage/plot/hyperbolic_triangle.py [8 tests, 2.53 s] sage t long src/sage/plot/plot3d/index_face_set.pyx [48 tests, 0.94 s] sage t long src/sage/plot/plot3d/texture.py [61 tests, 0.54 s] sage t long src/sage/plot/java3d.py [1 test, 0.05 s] sage t long src/sage/plot/plot3d/examples.py [4 tests, 0.59 s] sage t long src/sage/plot/plot3d/point_c.pxi [0 tests, 0.00 s] sage t long src/sage/plot/plot3d/help.pyx [0 tests, 0.00 s] sage t long src/sage/plot/plot3d/all.py [0 tests, 0.00 s] sage t long src/sage/plot/plot3d/__init__.py [0 tests, 0.00 s] sage t long src/sage/plot/all.py [0 tests, 0.00 s] sage t long src/sage/plot/__init__.py [0 tests, 0.00 s]  All tests passed!  Total time for all tests: 509.0 seconds cpu time: 813.9 seconds cumulative wall time: 1005.8 seconds ~/Installations/sagegit» sage version Sage Version 6.4.beta0, Release Date: 20140814 ~/Installations/sagegit» git s On branch ticket/16325 nothing to commit, working directory clean ~/Installations/sagegit» git l n 6 * eb27cd8  (HEAD, refs/heads/ticket/16325) simplify bordercolor 11 minutes ago <Punarbasu Purkayastha> * a7382e5  Merge branch 'u/ppurka/ticket/16325' of trac.sagemath.org:sage into ticket/16325 51 minutes ago <Punarbasu Purkayastha> \  * 2ecec6b  (refs/remotes/trac/u/ppurka/ticket/16325) bordercolor must not be a list. Fix this error. 3 months ago <Punarbasu Purkayastha>  * 2b1a65b  Enable fillcolor and bordercolor for implicit_plot 3 months ago <Punarbasu Purkayastha> *  07b385d  (refs/remotes/origin/develop, refs/heads/develop) Updated Sage version to 6.4.beta0 2 days ago <Volker Braun> *  974c1c1  Trac #16807: Overflow error in conversion Integer > FiniteFieldElement_pari_ffelt 3 days ago <Release Manager> \ \ %
comment:9 Changed 7 years ago by
 Commit changed from 2ecec6b91cce92fc326ed83e1bcb051eb2e4d203 to eb27cd89962322d64bd187760a919bf35aee33f4
comment:10 in reply to: ↑ 8 Changed 7 years ago by
I don't know why you are getting such errors. Things are working fine for me.
I'll have to try again, then  probably I made a mistake in setting up the branch or something. Won't be immediately. In any case, otherwise I liked the idea! Hopefully it was just something I did.
comment:11 Changed 7 years ago by
Not sure what happened before...
$ ./sage t src/sage/plot/contour_plot.py too many failed tests, not using stored timings Running doctests with ID 201408221606053f1b7b16. Doctesting 1 file. sage t src/sage/plot/contour_plot.py [123 tests, 25.81 s]  All tests passed!  Total time for all tests: 25.9 seconds cpu time: 25.5 seconds cumulative wall time: 25.8 seconds
Two requests:
 Add an example with a different color and fill color (for the user), if possible one where the function is not a symbolic expression (just to test that branch).
 Technically, I guess both occurrences of
string (default: ``blue``)
should bestring (default: ``'blue'``)
. Is that right? I know you will actually care about getting this right :)
Otherwise this is great, nice work. Interesting that it didn't work at all before.
comment:12 Changed 5 years ago by
 Branch changed from u/ppurka/ticket/16325 to public/graphics/implicit_plot_color_parameter16325
 Commit changed from eb27cd89962322d64bd187760a919bf35aee33f4 to 36d71e9af240eefb6bac34cc0ac044496eea5905
 Milestone changed from sage6.4 to sage7.3
 Reviewers changed from KarlDieter Crisman to KarlDieter Crisman, Travis Scrimshaw
 Status changed from needs_work to needs_review
I've made the changes you requested, as well as some other little tidying up of the doc in the file and fixing some trivially failing doctests.
New commits:
666a04b  Merge branch 'u/ppurka/ticket/16325' of trac.sagemath.org:sage into public/graphics/implicit_plot_color_parameter16325

36d71e9  Trivial doctest changes and some little doc fixes.

comment:13 Changed 5 years ago by
Thanks!
fille colors
otherwise, as long as this all still builds then should be positive review  I review your changes modulo that typo.
comment:14 Changed 5 years ago by
 Commit changed from 36d71e9af240eefb6bac34cc0ac044496eea5905 to 1da468df2cb825bc7c26637dc48a534aaffd7809
Branch pushed to git repo; I updated commit sha1. New commits:
1da468d  Fixing typo.

comment:15 Changed 5 years ago by
No problem. Done.
comment:16 Changed 5 years ago by
Wow! Thanks for taking care of this guys! I must have forgotten about this ticket.
Lately, I am unable to do any sage development because my laptop overheats and shuts down any time I do anything compute intensive.
comment:17 Changed 5 years ago by
 Cc paulmasson added
 Status changed from needs_review to needs_work
This file has half a dozen merge conflicts with 7.3.beta8, some of them in the code. I'm happy to review when fixed.
comment:18 Changed 5 years ago by
 Commit 1da468df2cb825bc7c26637dc48a534aaffd7809 deleted
Travis, I was going to resolve the merge conflicts here but the branch went missing sometime yesterday evening. Could you push it back up again? Thanks.
comment:19 Changed 5 years ago by
 Commit set to 9d7ba99a0b98a7478808a8c902d447b4ebc60612
Branch pushed to git repo; I updated commit sha1. New commits:
2b1a65b  Enable fillcolor and bordercolor for implicit_plot

2ecec6b  bordercolor must not be a list. Fix this error.

a7382e5  Merge branch 'u/ppurka/ticket/16325' of trac.sagemath.org:sage into ticket/16325

eb27cd8  simplify bordercolor

666a04b  Merge branch 'u/ppurka/ticket/16325' of trac.sagemath.org:sage into public/graphics/implicit_plot_color_parameter16325

36d71e9  Trivial doctest changes and some little doc fixes.

1da468d  Fixing typo.

9d7ba99  Merge branch 'develop' into public/graphics/implicit_plot_color_parameter16325

comment:20 Changed 5 years ago by
 Status changed from needs_work to needs_review
I managed to find a copy, and I also handled the merge conflict.
comment:21 Changed 5 years ago by
 Commit changed from 9d7ba99a0b98a7478808a8c902d447b4ebc60612 to 7e74f9e06040722a8b2a5532b548460c3029c804
comment:22 Changed 5 years ago by
Two doctests were failing which I fixed. They all pass now. New functionality for plots works as expected.
I added sphinx plots for new examples. Documentation builds.
Who acts as reviewer now?
comment:23 Changed 5 years ago by
 Milestone changed from sage7.3 to sage7.4
 Reviewers changed from KarlDieter Crisman, Travis Scrimshaw to KarlDieter Crisman, Travis Scrimshaw, Paul Masson
I can, and the only thing that I think needs to be done is removing the # long time
from the .. PLOT::
code (at least, I don't see the purpose of it being there).
comment:24 Changed 5 years ago by
 Commit changed from 7e74f9e06040722a8b2a5532b548460c3029c804 to daf974e16c14dd934407f9701cdbcd9ee1544b77
Branch pushed to git repo; I updated commit sha1. New commits:
daf974e  More cleanup

comment:25 Changed 5 years ago by
Removed # long time
from four ..PLOT::
s in the file.
comment:27 Changed 5 years ago by
 Branch changed from public/graphics/implicit_plot_color_parameter16325 to daf974e16c14dd934407f9701cdbcd9ee1544b77
 Resolution set to fixed
 Status changed from positive_review to closed
Sorry I'm adding myself to all these but not finding time to review them... I need to set aside a couple entire days after I finish grading finals just to review your plotting improvements, there are so many! (Which is good.)
New commits:
Enable fillcolor and bordercolor for implicit_plot
bordercolor must not be a list. Fix this error.