Opened 6 years ago
Closed 6 years ago
#17507 closed defect (fixed)
minor error with integral.n()
Reported by:  rws  Owned by:  

Priority:  minor  Milestone:  sage6.5 
Component:  calculus  Keywords:  
Cc:  kcrisman  Merged in:  
Authors:  Ralf Stephan, Nathann Cohen  Reviewers:  Nathann Cohen, Ralf Stephan 
Report Upstream:  N/A  Work issues:  
Branch:  5551787 (Commits, GitHub, GitLab)  Commit:  55517878d1460022fe3b027322ff847e01440233 
Dependencies:  Stopgaps: 
Description (last modified by )
sage: from sage.symbolic.integration.integral import definite_integral, indefinite_integral sage: definite_integral(x*y,x,0,1,hold=True) integrate(x*y, x, 0, 1) sage: _.n() ValueError: Integrand has wrong number of parameters
The commands are of course nonsense but the error is really misleading.
Change History (14)
comment:1 Changed 6 years ago by
 Milestone changed from sage6.5 to sageduplicate/invalid/wontfix
 Status changed from new to needs_review
comment:2 Changed 6 years ago by
 Status changed from needs_review to positive_review
comment:3 Changed 6 years ago by
 Description modified (diff)
 Milestone changed from sageduplicate/invalid/wontfix to sage6.5
 Priority changed from major to minor
 Status changed from positive_review to needs_work
 Summary changed from wrong output from definite_integral() to minor error with integral.n()
To be more exact, this ticket is just about the n()
comment:4 Changed 6 years ago by
 Cc kcrisman added
What kind of error would you suggest? InputError
, ValueError
... ?
comment:5 Changed 6 years ago by
 Branch set to u/rws/minor_error_with_integral_n__
comment:6 Changed 6 years ago by
 Commit set to 87fb1030cd271861aafe2fb42e58ab03df87e34a
I cannot find documentation about InputError
, so ValueError
is fine. Looking at the trigger condition however
if len(params) + 1 != len(vars): raise ValueError, "Integrand has wrong number of parameters"
suggests this is meant to catch problems with the params
argument to numerical_integral
. This is not given by the calling _evalf_
so maybe that should be checked upstream too to honor untold preconditions of numerical_integral
.
New commits:
87fb103  17507: change error msg, add doctest

comment:7 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:8 Changed 6 years ago by
 Status changed from needs_review to needs_info
Hello !
I think that this branch is ready to go, but I have a couple of questions to ask (I am new to this code):
 What would you think of an alternative error message: "The function to be integrated depends on 2 variables (x,y), and so cannot be integrated in one dimension. Please fix additional variables with the 'params' argument".
It took me some time to understand what exactly the problem was with the current error message, because I did not understand what 'parameters' meant.
 It seems dangerous to me that
params
is a list but not a dictionary. It seems difficult for the user to know for sure which variable will receive which value. Isn't that a problem ? Sorry, I really do not know the code.
If you agree to change the error message to the one I proposed, tell me and I will write a commit.
Nathann
comment:9 followups: ↓ 10 ↓ 11 Changed 6 years ago by
As to the message, please go ahead. The expression.variables()
returns a lexically sorted list but the function.arguments()
returns the arguments as they are given. This can get really confusing:
sage: f(x,a) = 1/(a+x^2) sage: f.variables() (a, x) sage: f.arguments() (x, a) sage: numerical_integral(f, 1, 2, max_points=100, params=[10]) (0.08148091340506758, 9.046198612979459e16) sage: numerical_integral(1/(a+x^2), 1, 2, max_points=100, params=[10]) (0.00985229644301163, 1.0938246356464044e16)
OTOH, giving a dictionary always involves the user more than a list. So maybe we should at least put a warning in the documentation about this. If you feel up to it, cvould you please?
comment:10 in reply to: ↑ 9 ; followup: ↓ 12 Changed 6 years ago by
 Branch changed from u/rws/minor_error_with_integral_n__ to public/minor_error_with_integral_n__
 Commit changed from 87fb1030cd271861aafe2fb42e58ab03df87e34a to 55517878d1460022fe3b027322ff847e01440233
 Status changed from needs_info to needs_review
Hello,
As to the message, please go ahead.
It is in the new commit.
The
expression.variables()
returns a lexically sorted list but thefunction.arguments()
returns the arguments as they are given. This can get really confusing:OTOH, giving a dictionary always involves the user more than a list.
What would you think of that: numerical_integral(x*y,0,1,y=3)
?
The 'dictionary' can also be built from args.
This being said, perhaps the "most simple and clean" prototype would be to refuse input with more than one variables, and ask the user to input:
sage: numerical_integral((x*y).subs(y=4),0,1) (2.0, 2.220446049250313e14)
This would not be very welcoming for beginners, though :/
So maybe we should at least put a warning in the documentation about this. If you feel up to it, cvould you please?
I would like to fix it, but I do not know if this 'param' argument is not used in other places of the symbolic functions, in which case it would call for a global change. I will write to sagedevel about that in a second.
I set the ticket in its current state as needs_review
: if we change the way to fix variables in this functions we will do so in another ticket.
Thanks,
Nathann
New commits:
4a78f46  trac #17507: Merged with 6.5.beta5

5551787  trac #17507: minor error with integral.n()

comment:11 in reply to: ↑ 9 Changed 6 years ago by
comment:12 in reply to: ↑ 10 Changed 6 years ago by
 Reviewers set to Nathann Cohen, Ralf Stephan
 Status changed from needs_review to positive_review
Replying to ncohen:
This being said, perhaps the "most simple and clean" prototype would be to refuse input with more than one variables, and ask the user to input:
sage: numerical_integral((x*y).subs(y=4),0,1) (2.0, 2.220446049250313e14)This would not be very welcoming for beginners, though
:/
Oh I don't think so at all. I like this very much.
Ticket looks good, tests ok. I take the liberty to fill in the fields.
comment:13 Changed 6 years ago by
Thanks :)
comment:14 Changed 6 years ago by
 Branch changed from public/minor_error_with_integral_n__ to 55517878d1460022fe3b027322ff847e01440233
 Resolution set to fixed
 Status changed from positive_review to closed
This should be fixed with #2787 so this is a duplicate.