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 )
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:
- apply trac-10489-slope-field-lambda-4-7-3.patch to the Sage library
Attachments (3)
Change History (24)
comment:1 Changed 9 years ago by
comment:2 follow-up: ↓ 4 Changed 9 years ago by
- 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
comment:3 Changed 9 years ago by
- 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: ↓ 5 Changed 9 years ago by
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
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
- Cc kcrisman added
comment:7 Changed 9 years ago by
- Keywords beginner added
Add the beginner keyword, as this seems like a good beginner review.
comment:8 Changed 9 years ago by
- 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)
comment:9 follow-up: ↓ 10 Changed 9 years ago by
- 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
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
#5553 is a related issue to the divide by zero issue.
comment:12 Changed 9 years ago by
The issue with the divide by zero warnings is #11208.
comment:13 Changed 8 years ago by
The most recent patch (12 mo ago) doesn't apply anymore. I will now upload an updated version of the same patch, for review.
comment:14 Changed 8 years ago by
- Status changed from needs_work to needs_review
comment:15 Changed 8 years ago by
- Keywords sd35.5 added
comment:16 Changed 8 years ago by
- 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
- 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:19 Changed 8 years ago by
They look correct to me.
comment:20 Changed 8 years ago by
- Milestone changed from sage-4.8 to sage-5.0
comment:21 Changed 8 years ago by
- Merged in set to sage-5.0.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
Did that ever work? I think the code assumes that a symbolic function is given: