Opened 9 years ago

Closed 8 years ago

#9744 closed defect (fixed)

implicit_plot fill option fills entire plot

Reported by: jason Owned by: jason, was
Priority: critical Milestone: sage-5.0
Component: graphics Keywords:
Cc: kcrisman Merged in: sage-5.0.beta6
Authors: Jason Grout, Michael Boratko, Benjamin Jones Reviewers: Karl-Dieter Crisman, Benjamin Jones
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by kcrisman)

I was browsing the docs and noticed this example completely fills the plot black:

x,y = var('x,y')
f(x,y) = x^2 + y^2 - 2
implicit_plot(f, (-3, 3), (-3, 3),fill=True).show(aspect_ratio=1)

The docs say it should fill the region f(x)<0.


Apply: trac_9744_v2.patch and trac_9744-reviewer.patch.

Attachments (4)

implicit-plot-fill.patch (1.0 KB) - added by jason 9 years ago.
trac_9744.patch (1.3 KB) - added by mboratko 8 years ago.
trac_9744_v2.patch (2.3 KB) - added by benjaminfjones 8 years ago.
fixes filled implicit plotting with lambdas
trac_9744-reviewer.patch (2.2 KB) - added by kcrisman 8 years ago.
reviewer patch

Download all attachments as: .zip

Change History (31)

Changed 9 years ago by jason

comment:1 Changed 9 years ago by jason

  • Status changed from new to needs_work

Here is a rough patch which conflicts with #9654, but gives an idea of how this issue could be resolved.

comment:2 Changed 8 years ago by kcrisman

  • Cc kcrisman added

Hmm, I feel like there's another ticket about this... but anyway, this is important to fix.

comment:3 Changed 8 years ago by kcrisman

I think it's #8529 that you meant. Anyway, is there anything wrong with this rough patch? I don't see what the problem might be.

Changed 8 years ago by mboratko

comment:4 Changed 8 years ago by mboratko

This is just a modification of Jason's fix to work with the current version. I had to change 'contour' to 'contours', and also remove the 'cmap' option in order for it to work (you can see what I mean by comparing the two patches). I'm pretty new to this, so I'm not sure if this is the appropriate fix or not, but it has the desired effect.

comment:5 Changed 8 years ago by kcrisman

  • Authors set to Jason Grout, mboratko
  • Status changed from needs_work to needs_review

Thanks for this; this would be good for Sage Days 35.5. Please change your name in the Author section to your real name, and (if you don't mind) add it to the main Trac page!

comment:6 Changed 8 years ago by mboratko

  • Authors changed from Jason Grout, mboratko to Jason Grout, Michael Boratko

comment:7 Changed 8 years ago by benjaminfjones

  • Reviewers set to Benjamin Jones

I ran into this bug and thought I would review the patch here. The patch applies cleanly to 4.8.alpha6 (I'm still waiting for 4.8.rc0 to build). Doctests on sage/plot/contour_plot.py pass. More importantly, I went through all the docstring examples for implicit_plot by hand in the notebook to make sure they look correct, and they all do so that's great!

However, when I try the following (a modification of one of the doctests):

sage: def mandel(n):
...       c = polygen(CDF, 'c')
...       z = 0
...       for i in range(n):
...           z = z*z + c
...       def f(x, y):
...           val = z(CDF(x, y))
...           return val.norm() - 4
...       return f
sage: implicit_plot(mandel(1), (-3, 3), (-3, 3), fill=True)

I get an error:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "_sage_input_21.py", line 10, in <module>
    exec compile(u'open("___code___.py","w").write("# -*- coding: utf-8 -*-\\n" + _support_.preparse_worksheet_cell(base64.b64decode("aW1wbGljaXRfcGxvdChtYW5kZWwoMSksICgtMywgMyksICgtMywgMyksIGZpbGw9VHJ1ZSk="),globals())+"\\n"); execfile(os.path.abspath("___code___.py"))
  File "", line 1, in <module>
    
  File "/tmp/tmpp9xXZj/___code___.py", line 3, in <module>
    exec compile(u'implicit_plot(mandel(_sage_const_1 ), (-_sage_const_3 , _sage_const_3 ), (-_sage_const_3 , _sage_const_3 ), fill=True)
  File "", line 1, in <module>
    
  File "/home/jonesbe/sage/sage-4.8.alpha6/local/lib/python2.6/site-packages/sage/misc/decorators.py", line 534, in wrapper
    return func(*args, **options)
  File "/home/jonesbe/sage/sage-4.8.alpha6/local/lib/python2.6/site-packages/sage/plot/contour_plot.py", line 609, in implicit_plot
    return region_plot(f<0, xrange, yrange, borderwidth=linewidths, borderstyle=linestyles, **options)
  File "/home/jonesbe/sage/sage-4.8.alpha6/local/lib/python2.6/site-packages/sage/misc/decorators.py", line 534, in wrapper
    return func(*args, **options)
  File "/home/jonesbe/sage/sage-4.8.alpha6/local/lib/python2.6/site-packages/sage/plot/contour_plot.py", line 726, in region_plot
    for func in g],dtype=float)
  File "/home/jonesbe/sage/sage-4.8.alpha6/local/lib/python2.6/site-packages/sage/plot/contour_plot.py", line 783, in <lambda>
    return lambda x,y: -1 if f(x,y) else 1
TypeError: 'bool' object is not callable

Any idea what's going on here?

comment:8 Changed 8 years ago by kcrisman

  • Priority changed from major to critical
  • Reviewers changed from Benjamin Jones to Benjamin Jones, Karl-Dieter Crisman
  • Status changed from needs_review to needs_work
  • Work issues set to fix lambda functions with fill

BFJ, I assume this means positive review apart from this error?

Huh, yeah, this definitely means 'needs work'. Even though the functionality didn't work at all before, lambdas shouldn't break too many things, and this is a very natural change to make. The problem is in equify (see the very bottom of the file.

sage: c = polygen(CDF,'c')
sage: z = 0
sage: for i in range(1):
    z = z*z + c
....: 
sage: def g(x,y):
    val = z(CDF(x, y))
    return val.norm() - 4
....: 
sage: implicit_plot(g,(-3,3),(-3,3))  # looks fine, no surprise

sage: sage.plot.contour_plot.equify(g)  # no problem, so plotting should work with fill
<function <lambda> at 0x10df9d578>
sage: implicit_plot(g,(-3,3),(-3,3),fill=True) # error above

Got it. The problem is

f<0

in region_plot. In the case I just did,

sage: g<0
True

so we are now plotting True instead of g, which of course makes the error message quite understandable.

I tried a few things to see if I could fix it in a few minutes, but I think the "right" answer might be more complicated.

Unless we think we should just fix this and put in a warning about lambdas being broken? Seems poor form.

comment:9 Changed 8 years ago by benjaminfjones

Yeah, seems like we should make it work for lambda's since implicit_plot works just fine, it's only when you set fill=True that you get a problem. I'll take a crack at it this afternoon.

comment:10 Changed 8 years ago by benjaminfjones

  • Authors changed from Jason Grout, Michael Boratko to Jason Grout, Michael Boratko, Benjamin Jones
  • Reviewers changed from Benjamin Jones, Karl-Dieter Crisman to Karl-Dieter Crisman
  • Status changed from needs_work to needs_review

I uploaded trac_9744_v2.patch which solves the main problem by calling region_plot(lambda x,y: f(x,y)<0, ...) instead of region_plot(f<0, ...)

...but it introduces a strange artefact. Try the following (this doesn't require applying the patch).

sage: region_plot(x^2+y^2-2<0, (x,-3,3), (y,-3,3))
# looks normal
sage: region_plot(lambda x,y: x^2+y^2-2<0, (x,-3,3), (y,-3,3))
# looks ragged around the edges
sage: region_plot(lambda x,y: x^2+y^2-2<0, (x,-3,3), (y,-3,3), plot_points=200)
# better, but not great
sage: region_plot(lambda x,y: x^2+y^2-2<0, (x,-3,3), (y,-3,3), plot_points=500)
# looks about like the original, but takes a long time.

comment:11 Changed 8 years ago by benjaminfjones

  • Description modified (diff)
  • Work issues fix lambda functions with fill deleted

The following doctest that is currently in region_plot has the same artefact:

region_plot(lambda x,y: x^2+y^2<1 or x<y, (x,-2,2), (y,-2,2))

I uploaded a new, new patch that only has the artefact when a python function is passed to implicit_plot in which case the user should increase the point count appropriately. I added a doctest to that effect as well.

With the patch applied, compare:

sage: x,y=var('x,y')
sage: implicit_plot(x^2+y^2-2, (x,-3,3), (y,-3,3), fill=True)
sage: g = lambda x,y: x^2+y^2-2
sage: implicit_plot(g,  (x,-3,3), (y,-3,3), fill=True)

comment:12 Changed 8 years ago by kcrisman

I'm between classes now, but I'll check it out later. In the meantime, what is going on with the new code? When is the last line

return contour_plot(f, xrange, yrange, linewidths=linewidths, linestyles=linestyles, **options)

ever reached?

Also,

if options.pop('fill'):

will give a KeyError if 'fill' isn't defined in the options dictionary. Could that happen? If so, you may want to take the previous entry

if 'color' in options: 

and go from there... this may not be a problem, but it's probably wisest to guard against it.

comment:13 follow-up: Changed 8 years ago by benjaminfjones

The if options.pop('fill'): is okay because of the @options decorator before def implicit_plot(...):

@options(plot_points=150, contours=(0,0), fill=False, cmap=["blue"])

You're correct about that last line, somehow I let that slip through. I updated the patch. I think it's more readable and less redundant now.

comment:14 in reply to: ↑ 13 Changed 8 years ago by kcrisman

The if options.pop('fill'): is okay because of the @options decorator before def implicit_plot(...):

I figured, but was too rushed to check.

x,y = var('x,y')
f(x,y) = x^2 + y^2 - 2
implicit_plot(f, (-3, 3), (-3, 3),fill=True).show(aspect_ratio=1)

It works!

Let me just look at a few more things. I think I'll want to add something to the other example you give, with the region plot lambda, so that it looks nicer in the doc.


I was going to complain about artefact but then read this. Harrumph.

comment:15 Changed 8 years ago by benjaminfjones

That sounds good. Go ahead and change artefact if you want. I'll admit I'm of the generation whose spelling abilities were destroyed by the spell checker.

I was thinking about opening a new ticket to improve results of region_plot when passing a lambda function, but I don't know enough about why the "artefacts" occur. Any thoughts?

comment:16 Changed 8 years ago by kcrisman

No, I don't know - I haven't plotted too many lambda functions recently, of course.

I'm going to fix a few other doc things. One that looks like a change, but isn't, is removing the variable declaration - but this is not needed with f(x,y) notation.

comment:17 Changed 8 years ago by kcrisman

  • Description modified (diff)

comment:18 Changed 8 years ago by benjaminfjones

Reviewer patch looks good to me. Thanks.

comment:19 Changed 8 years ago by kcrisman

Wow, I wasn't even done reviewing my own reviewer patch :) I got hung up on trying to decide whether to add any more # long times, but decided it wasn't worth it.

The code currently allows this.

sage: f(x,y) = x^2 + y^2 - 2
sage: implicit_plot(f, (-3, 3), (-3, 3),fill='55').show(aspect_ratio=1)  # nice graph is filled

Should we maybe raise an error if this happens? Given that one can ask for all kinds of fills in regular plots, not just True or False, perhaps this should be disallowed.

comment:20 Changed 8 years ago by kcrisman

  • Reviewers changed from Karl-Dieter Crisman to Karl-Dieter Crisman, Benjamin Jones

Also, it's okay to be a reviewer and an author - you definitely have done both here!

comment:21 Changed 8 years ago by benjaminfjones

I updated my patch to make the code more readable at the end of implicit_plot and to raise an error if fill option is passed and does not equal either True or False.

The reviewer patch still applies on top (with a tiny bit of fuzz).

comment:22 Changed 8 years ago by kcrisman

I think you might want to do

if foo is True

not

if foo == True

for reasons that escape me right now.

comment:23 Changed 8 years ago by jason

'is' does a pointer comparison, which is much faster and works because True is a singleton. == does a eq comparison, which is much slower. When comparing to True, False, or None, you should use 'is' to be much faster.

Changed 8 years ago by benjaminfjones

fixes filled implicit plotting with lambdas

comment:24 Changed 8 years ago by benjaminfjones

Ah, good point. The new patch should do the job.

comment:25 Changed 8 years ago by kcrisman

I can't think of any other reasons to hold this up. I've uploaded a new reviewer patch testing the error raised. I tested the looks of the implicit plots in schemes and rings, even. I think we're done here.

Changed 8 years ago by kcrisman

reviewer patch

comment:26 Changed 8 years ago by kcrisman

  • Status changed from needs_review to positive_review

comment:27 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.0.beta6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.