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: sage-7.4
Component: graphics Keywords:
Cc: kcrisman, paulmasson Merged in:
Authors: Punarbasu Purkayastha Reviewers: Karl-Dieter Crisman, Travis Scrimshaw, Paul Masson
Report Upstream: N/A Work issues:
Branch: daf974e (Commits, GitHub, GitLab) Commit: daf974e16c14dd934407f9701cdbcd9ee1544b77
Dependencies: Stopgaps:

Status badges

Description (last modified by ppurka)

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.

sage-support reference

Change History (27)

comment:1 Changed 7 years ago by ppurka

  • Description modified (diff)

comment:2 Changed 7 years ago by ppurka

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

  • Status changed from new to needs_review

comment:4 Changed 7 years ago by kcrisman

  • Cc kcrisman added
  • Commit set to 2ecec6b91cce92fc326ed83e1bcb051eb2e4d203

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:

2b1a65bEnable fillcolor and bordercolor for implicit_plot
2ecec6bbordercolor must not be a list. Fix this error.

comment:5 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:6 Changed 7 years ago by kcrisman

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 kcrisman

  • Reviewers set to Karl-Dieter Crisman
  • Status changed from needs_review to needs_work

comment:8 follow-up: Changed 7 years ago by ppurka

I don't know why you are getting such errors. Things are working fine for me.

~/Installations/sage-git» sage -tp --long src/sage/plot
too few successful tests, not using stored timings
Running doctests with ID 2014-08-16-18-13-09-745f1adf.
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/sage-git» sage --version
Sage Version 6.4.beta0, Release Date: 2014-08-14
~/Installations/sage-git» git s
On branch ticket/16325
nothing to commit, working directory clean

~/Installations/sage-git» 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>
|\ \  %                                  
Last edited 7 years ago by ppurka (previous) (diff)

comment:9 Changed 7 years ago by git

  • Commit changed from 2ecec6b91cce92fc326ed83e1bcb051eb2e4d203 to eb27cd89962322d64bd187760a919bf35aee33f4

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

a7382e5Merge branch 'u/ppurka/ticket/16325' of trac.sagemath.org:sage into ticket/16325
eb27cd8simplify bordercolor

comment:10 in reply to: ↑ 8 Changed 7 years ago by kcrisman

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 kcrisman

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 2014-08-22-16-06-05-3f1b7b16.
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 be string (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 tscrim

  • Authors set to Punarbasu Purkayastha
  • Branch changed from u/ppurka/ticket/16325 to public/graphics/implicit_plot_color_parameter-16325
  • Commit changed from eb27cd89962322d64bd187760a919bf35aee33f4 to 36d71e9af240eefb6bac34cc0ac044496eea5905
  • Milestone changed from sage-6.4 to sage-7.3
  • Reviewers changed from Karl-Dieter Crisman to Karl-Dieter 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:

666a04bMerge branch 'u/ppurka/ticket/16325' of trac.sagemath.org:sage into public/graphics/implicit_plot_color_parameter-16325
36d71e9Trivial doctest changes and some little doc fixes.

comment:13 Changed 5 years ago by kcrisman

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 git

  • Commit changed from 36d71e9af240eefb6bac34cc0ac044496eea5905 to 1da468df2cb825bc7c26637dc48a534aaffd7809

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

1da468dFixing typo.

comment:15 Changed 5 years ago by tscrim

No problem. Done.

comment:16 Changed 5 years ago by ppurka

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 paulmasson

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

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

  • Commit set to 9d7ba99a0b98a7478808a8c902d447b4ebc60612

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

2b1a65bEnable fillcolor and bordercolor for implicit_plot
2ecec6bbordercolor must not be a list. Fix this error.
a7382e5Merge branch 'u/ppurka/ticket/16325' of trac.sagemath.org:sage into ticket/16325
eb27cd8simplify bordercolor
666a04bMerge branch 'u/ppurka/ticket/16325' of trac.sagemath.org:sage into public/graphics/implicit_plot_color_parameter-16325
36d71e9Trivial doctest changes and some little doc fixes.
1da468dFixing typo.
9d7ba99Merge branch 'develop' into public/graphics/implicit_plot_color_parameter-16325

comment:20 Changed 5 years ago by tscrim

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

  • Commit changed from 9d7ba99a0b98a7478808a8c902d447b4ebc60612 to 7e74f9e06040722a8b2a5532b548460c3029c804

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

fd306fbFix doctests and minor cleanup
7e74f9eAdd sphinx plots and cleanup

comment:22 Changed 5 years ago by paulmasson

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 tscrim

  • Milestone changed from sage-7.3 to sage-7.4
  • Reviewers changed from Karl-Dieter Crisman, Travis Scrimshaw to Karl-Dieter 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 git

  • Commit changed from 7e74f9e06040722a8b2a5532b548460c3029c804 to daf974e16c14dd934407f9701cdbcd9ee1544b77

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

daf974eMore cleanup

comment:25 Changed 5 years ago by paulmasson

Removed # long time from four ..PLOT::s in the file.

comment:26 Changed 5 years ago by tscrim

  • Status changed from needs_review to positive_review

Thanks.

comment:27 Changed 5 years ago by vbraun

  • Branch changed from public/graphics/implicit_plot_color_parameter-16325 to daf974e16c14dd934407f9701cdbcd9ee1544b77
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.