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: sage-6.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:

Status badges

Description (last modified by rws)

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 rws

  • Milestone changed from sage-6.5 to sage-duplicate/invalid/wontfix
  • Status changed from new to needs_review

This should be fixed with #2787 so this is a duplicate.

comment:2 Changed 6 years ago by rws

  • Status changed from needs_review to positive_review

comment:3 Changed 6 years ago by rws

  • Description modified (diff)
  • Milestone changed from sage-duplicate/invalid/wontfix to sage-6.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 kcrisman

  • Cc kcrisman added

What kind of error would you suggest? InputError, ValueError ... ?

comment:5 Changed 6 years ago by rws

  • Branch set to u/rws/minor_error_with_integral_n__

comment:6 Changed 6 years ago by rws

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

87fb10317507: change error msg, add doctest

comment:7 Changed 6 years ago by rws

  • Authors set to Ralf Stephan
  • Status changed from needs_work to needs_review

comment:8 Changed 6 years ago by ncohen

  • 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 follow-ups: Changed 6 years ago by rws

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.046198612979459e-16)
sage: numerical_integral(1/(a+x^2), 1, 2, max_points=100, params=[10])
(0.00985229644301163, 1.0938246356464044e-16)

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 ; follow-up: Changed 6 years ago by ncohen

  • 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 the function.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.220446049250313e-14)

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 sage-devel 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:

4a78f46trac #17507: Merged with 6.5.beta5
5551787trac #17507: minor error with integral.n()
Version 0, edited 6 years ago by ncohen (next)

comment:12 in reply to: ↑ 10 Changed 6 years ago by rws

  • Authors changed from Ralf Stephan to Ralf Stephan, Nathann Cohen
  • 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.220446049250313e-14)

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 ncohen

Thanks :-)

comment:14 Changed 6 years ago by vbraun

  • Branch changed from public/minor_error_with_integral_n__ to 55517878d1460022fe3b027322ff847e01440233
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.