Opened 9 years ago

Closed 8 years ago

#10489 closed enhancement (fixed)

plot_slope_field broken

Reported by: ManDay Owned by: jason, was
Priority: major Milestone: sage-5.0
Component: graphics Keywords: plot_slope_field beginner sd35.5
Cc: kcrisman Merged in: sage-5.0.beta1
Authors: Ryan Grout, Jason Grout, Nathan Carter Reviewers: Aly Deines, Ryan Grout, Benjamin Jones
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by benjaminfjones)

plot_slope_field( lambda x,y: x+y,(x,-3,4),(y,-2,2) );

results in

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "_sage_input_15.py", line 10, in <module>
    exec compile(u'open("___code___.py","w").write("# -*- coding: utf-8 -*-\\n" + _support_.preparse_worksheet_cell(base64.b64decode("cGxvdF9zbG9wZV9maWVsZCggbGFtYmRhIHgseTogeCt5LCh4LC0zLDQpLCh5LC0yLDIpICk7"),globals())+"\\n"); execfile(os.path.abspath("___code___.py"))
  File "", line 1, in <module>
    
  File "/tmp/tmp639i0K/___code___.py", line 3, in <module>
    exec compile(u'plot_slope_field( lambda x,y: x+y,(x,-_sage_const_3 ,_sage_const_4 ),(y,-_sage_const_2 ,_sage_const_2 ) );
  File "", line 1, in <module>
    
  File "/homes/sageserv/sage-4.5.1-Solaris_10_SPARC/local/lib/python2.6/site-packages/sage/plot/plot_field.py", line 220, in plot_slope_field
    norm = sqrt((f**2+1))
TypeError: unsupported operand type(s) for ** or pow(): 'function' and 'int'

To release manager:

Attachments (3)

10489_slope_field_lambda.patch (1.3 KB) - added by jason 9 years ago.
10489_slope_field_lambda_reviewer.patch (1.3 KB) - added by ryan 9 years ago.
rebased against sage-4.6.1
trac-10489-slope-field-lambda-4-7-3.patch (1.2 KB) - added by ncarter 8 years ago.
Updating previous patch to newer Sage version

Download all attachments as: .zip

Change History (24)

comment:1 Changed 9 years ago by jason

Did that ever work? I think the code assumes that a symbolic function is given:

sage: f(x,y)=x+y
sage: plot_slope_field(f,(x,-3,4),(y,-2,2) )

comment:2 follow-up: Changed 9 years ago by jason

  • Milestone set to sage-4.6.2
  • Type changed from defect to enhancement

Changing to enhancement, since I don't think the code was ever designed to handle a python lambda function. I suggest changing the code to see if we have a symbolic expression or a python function. If a python function, then create a new lambda function which normalizes the vectors.

Changed 9 years ago by jason

comment:3 Changed 9 years ago by jason

  • Status changed from new to needs_review

Attached patch adds this feature to plot_slope_field. Someone just needs to review it!

comment:4 in reply to: ↑ 2 ; follow-up: Changed 9 years ago by ManDay

Thank you. I figured this out by now but you had already replied. I just think this should be a consistent behaviour through all plotting functions (unless the nature of the function itsself demands otherwise - and then it should be made very explicit). I don't think that some function should be able to handle both, some only the prior and some only the latter.

comment:5 in reply to: ↑ 4 Changed 9 years ago by jason

Replying to ManDay:

Thank you. I figured this out by now but you had already replied. I just think this should be a consistent behaviour through all plotting functions (unless the nature of the function itsself demands otherwise - and then it should be made very explicit). I don't think that some function should be able to handle both, some only the prior and some only the latter.

I agree! Feel free to review the patch.

comment:6 Changed 9 years ago by kcrisman

  • Cc kcrisman added

comment:7 Changed 9 years ago by jason

  • Keywords beginner added

Add the beginner keyword, as this seems like a good beginner review.

comment:8 Changed 9 years ago by aly.deines

  • Reviewers set to Aly Deines
  • Status changed from needs_review to needs_work

Running sage-4.6.1.rc0 on sage.math the patch did not apply:

adeines@sage:~/sage-4.6.1.rc0/devel/sage$ hg qpush applying 10489_slope_field_lambda.patch patching file sage/plot/plot_field.py Hunk #1 FAILED at 218 1 out of 1 hunks FAILED -- saving rejects to file sage/plot/plot_field.py.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh 10489_slope_field_lambda.patch adeines@sage:~/sage-4.6.1.rc0/devel/sage$ vi sage/plot/plot_field.py.rej

--- plot_field.py +++ plot_field.py @@ -219,10 +219,21 @@

sage: x,y = var('x y') sage: plot_slope_field(sin(x+y)+cos(x+y), (x,-3,3), (y,-3,3))

+ + Plot a slope field using a lambda function:: + + sage: plot_slope_field(lambda x,y: x+y, (-2,2), (-2,2)) +

""" slope_options = {'headaxislength': 0, 'headlength': 0, 'pivot': 'middle'} slope_options.update(kwds)

from sage.functions.all import sqrt

  • norm = sqrt((f2+1))
  • return plot_vector_field((1/norm, f/norm), xrange, yrange, slope_options)

+ from inspect import isfunction + if isfunction(f): + norm_inverse=lambda x,y: 1/sqrt(f(x,y)2+1) + f_normalized=lambda x,y: f(x,y)*norm_inverse(x,y) + else: + norm_inverse = 1/sqrt((f2+1)) + f_normalized=f*norm_inverse + return plot_vector_field((norm_inverse, f_normalized), xrange, yrange, slope_options)

Changed 9 years ago by ryan

rebased against sage-4.6.1

comment:9 follow-up: Changed 9 years ago by ryan

  • Reviewers changed from Aly Deines to Aly Deines, Ryan Grout

rebased patch against sage-4.6.1 Should apply cleanly now.

All tests passed on my machine. However, plotting the doctest gave several warnings before finally plotting the function. Should we maybe check for divide by zero cases or other such cases? {{{Warning: divide by zero encountered in divide Warning: invalid value encountered in multiply Warning: invalid value encountered in multiply Warning: divide by zero encountered in divide Warning: invalid value encountered in multiply Warning: invalid value encountered in multiply}}}

comment:10 in reply to: ↑ 9 Changed 9 years ago by jason

Replying to ryan:

rebased patch against sage-4.6.1 Should apply cleanly now.

All tests passed on my machine. However, plotting the doctest gave several warnings before finally plotting the function. Should we maybe check for divide by zero cases or other such cases?

Yes, but that is a separate issue from this ticket, so we should open a new ticket for that. In fact, I seem to remember that a ticket was opened several years ago for the divide by zero issue.

comment:11 Changed 9 years ago by jason

#5553 is a related issue to the divide by zero issue.

comment:12 Changed 9 years ago by kcrisman

The issue with the divide by zero warnings is #11208.

comment:13 Changed 8 years ago by ncarter

The most recent patch (12 mo ago) doesn't apply anymore. I will now upload an updated version of the same patch, for review.

Changed 8 years ago by ncarter

Updating previous patch to newer Sage version

comment:14 Changed 8 years ago by ncarter

  • Status changed from needs_work to needs_review

comment:15 Changed 8 years ago by ncarter

  • Keywords sd35.5 added

comment:16 Changed 8 years ago by benjaminfjones

  • Description modified (diff)
  • Reviewers changed from Aly Deines, Ryan Grout to Aly Deines, Ryan Grout, Benjamin Jones
  • Status changed from needs_review to positive_review

comment:17 Changed 8 years ago by benjaminfjones

  • Description modified (diff)

The patch trac-10489-slope-field-lambda-4-7-3.patch applies cleanly to Sage-4.8.alpha6, all doctests pass, the patch looks great.

Positive review.

comment:18 Changed 8 years ago by kcrisman

  • Authors set to Ryan Grout, Jason Grout, Nathan Carter

Assuming that the plots look great as well :)

comment:19 Changed 8 years ago by benjaminfjones

They look correct to me.

comment:20 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-4.8 to sage-5.0

comment:21 Changed 8 years ago by jdemeyer

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