Opened 8 years ago

Closed 7 years ago

#14766 closed defect (fixed)

Fix Python int problem with exp_integral

Reported by: kcrisman Owned by: burcin
Priority: major Milestone: sage-6.4
Component: symbolics Keywords:
Cc: benjaminfjones, burcin, eviatarbach Merged in:
Authors: Jeroen Demeyer Reviewers: Ralf Stephan
Report Upstream: N/A Work issues:
Branch: 68c545a (Commits, GitHub, GitLab) Commit: 68c545a5edb07ea87adea5850a143195723dfced
Dependencies: #17130 Stopgaps:

Status badges

Description

In #11143 we weren't careful about Python ints. There are lots of examples of this, so one will want to go through the whole file. 

sage: exp_integral_e(int(3),0)
0
sage: exp_integral_e(3,0)
1/2

Change History (11)

comment:1 Changed 8 years ago by kcrisman

  • Type changed from PLEASE CHANGE to defect

comment:2 Changed 8 years ago by jdemeyer

  • Cc changed from benjaminfjones,burcin, to benjaminfjones, burcin
  • Milestone changed from sage-5.11 to sage-5.12

comment:3 Changed 8 years ago by eviatarbach

  • Cc eviatarbach added

I'm guessing this occurs in other files too.

I think it's unreasonable to expect everyone adding symbolic functions to worry about this, and certainly users to be aware of it; maybe we could make BuiltinFunction.__call__ automatically convert int to Integer? Or have it raise an error if an int is in the parameters? Allowing int into _eval_ causes other problems too, such as getting expressions with int pyobjects in them, making arithmetic slower, etc.

At the very least, we could temporarily add from __future__ import division in all files that implement symbolic functions; then exact answers wouldn't be returned in all cases where they should, but at least mathematically wrong answers wouldn't occur.

comment:4 Changed 8 years ago by benjaminfjones

I think my preference would be either raising a TypeError (and clearly documenting what the allowed types are) or doing Python 3 style division. I don't like the idea of adding an implicit coercion int -> Integer.

If we do type checking in BuiltinFunction.__call__, we should make sure that there isn't a significant performance penalty.

comment:5 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:6 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:7 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:8 Changed 7 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/14766
  • Created changed from 06/18/13 17:37:43 to 06/18/13 17:37:43
  • Modified changed from 08/10/14 16:51:03 to 08/10/14 16:51:03

comment:9 Changed 7 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Commit set to 68c545a5edb07ea87adea5850a143195723dfced
  • Dependencies set to #17130
  • Status changed from new to needs_review

New commits:

​6d10729Simplify numerical evaluation of BuiltinFunctions
​b6e1ed4Merge remote-tracking branches 'origin/u/jdemeyer/ticket/17131' and 'origin/u/jdemeyer/ticket/17133' into ticket/17130
​382f97aMerge branch 'u/jdemeyer/ticket/17130' of trac.sagemath.org:sage into 6.5beta1
​726598917130: reviewer's patch: fix typo
​c47dbd4Fix Trac #17328 again in a better way
​a486db2Call the factorial() method of an Integer
​68c545aFix bugs due to Python 2 int division

comment:10 Changed 7 years ago by rws

  • Reviewers set to Ralf Stephan
  • Status changed from needs_review to positive_review

Looks fine and passes tests in functions/.

comment:11 Changed 7 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/14766 to 68c545a5edb07ea87adea5850a143195723dfced
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.