Opened 11 years ago

Closed 11 years ago

#7748 closed defect (fixed)

Make incomplete gamma function symbolic

Reported by: kcrisman Owned by: zimmerma
Priority: minor Milestone: sage-4.4
Component: symbolics Keywords:
Cc: mvngu Merged in: sage-4.4.alpha0
Authors: Flavia Stan, Burcin Erocal Reviewers: Paul Zimmermann
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

We don't appear to have this, though perhaps it is in Pynac/Ginac? already. We also need to translate Maxima's gamma_incomplete to our gamma_inc or incomplete_gamma as part of this ticket.

Attachments (9)

trac_7748-exp_integral.patch (7.5 KB) - added by fstan 11 years ago.
Make Ei symbolic.
trac_7748-incomplete_gamma.patch (6.5 KB) - added by fstan 11 years ago.
Make incomplete gamma function symbolic.
trac_7748-exp_integral_ver2.patch (7.9 KB) - added by fstan 11 years ago.
Added derivative method.
trac_7748-incomplete_gamma_ver2.patch (7.2 KB) - added by fstan 11 years ago.
Fixed tests from complex_number.pyx
trac_7748-exp_integral_ver2.4.3.3.alpha1.patch (7.9 KB) - added by burcin 11 years ago.
rebased to 4.3.3.alpha1
trac_7748-incomplete_gamma_ver2.4.3.3.alpha1.patch (7.2 KB) - added by burcin 11 years ago.
rebased to 4.3.3.alpha1
trac_7748-gamma_wrapper.patch (8.6 KB) - added by burcin 11 years ago.
wrapper for gamma and incomplete gamma
trac_7748-gamma_wrapper.2.patch (8.9 KB) - added by burcin 11 years ago.
wrapper for gamma and incomplete gamma
trac_7748-gamma_wrapper.3.patch (10.0 KB) - added by burcin 11 years ago.
fix new doctest failure

Download all attachments as: .zip

Change History (28)

comment:1 follow-up: Changed 11 years ago by burcin

After a quick look at the GiNaC sources, I don't think there is an implementation of the incomplete gamma function in there.

Numerical evaluation of this can be done with the gammainc() function of mpmath.

The conversion should just work, if the conversions dictionary argument of BuiltinFunction.__init__ contains maxima='gamma_incomplete'.

comment:2 in reply to: ↑ 1 Changed 11 years ago by kcrisman

Replying to burcin:

After a quick look at the GiNaC sources, I don't think there is an implementation of the incomplete gamma function in there.

Okay.

Numerical evaluation of this can be done with the gammainc() function of mpmath.

But will it give us symbolic evaluation ala gamma_inc(1,1)=1/e? That would be best.

The conversion should just work, if the conversions dictionary argument of BuiltinFunction.__init__ contains maxima='gamma_incomplete'.

Yes, of course, but we still have to do it :)

comment:3 Changed 11 years ago by was

Numerical evaluation of this can be done with the gammainc() function of mpmath.

Doesn't PARI also have an incomplete gamma implementation (it's used by RR and CC, I think)?

Changed 11 years ago by fstan

Make Ei symbolic.

Changed 11 years ago by fstan

Make incomplete gamma function symbolic.

comment:4 Changed 11 years ago by fstan

  • Authors set to Flavia Stan
  • Status changed from new to needs_review

The 2 patches should make the incomplete gamma function symbolic. The exponential integral Ei was used in its symbolic evaluation, therefore it was also made symbolic.

comment:5 follow-ups: Changed 11 years ago by zimmerma

I tried the exponential integral patch and it works ok. However I don't understand the changes in random_tests.py. Also the following could be evaluated:

sage: diff(Ei(x),x)
D[0](Ei)(x)
sage: integrate(Ei(x),x)
integrate(Ei(x), x)

Should this be in a separate ticket?

comment:6 in reply to: ↑ 5 Changed 11 years ago by kcrisman

Replying to zimmerma:

I tried the exponential integral patch and it works ok. However I don't understand the changes in random_tests.py.

This makes a 'random' symbolic expression, and when we add new symbolic functions (and sometimes when we change them) this doctest needs to be changed, not because anything bad happened.

comment:7 in reply to: ↑ 5 Changed 11 years ago by fstan

  • Status changed from needs_review to needs_work

Replying to zimmerma:

I tried the exponential integral patch and it works ok. However I don't understand the changes in random_tests.py. Also the following could be evaluated:

sage: diff(Ei(x),x)
D[0](Ei)(x)
sage: integrate(Ei(x),x)
integrate(Ei(x), x)

Should this be in a separate ticket?

Thank you for the review. I'll include the derivative and submit again. Integration looks more complicated.

Greetings,
Flavia

comment:8 follow-up: Changed 11 years ago by zimmerma

  • Owner changed from burcin to zimmerma

I tried sage -t * with both patches, and I get one additional failure with respect to those I get due to #7773:

tarte% sage -t "rings/complex_number.pyx"
sage -t  "rings/complex_number.pyx"                         
**********************************************************************
File "/usr/local/sage-4.3-core2/devel/sage-main/sage/rings/complex_number.pyx", line 1611:
    sage: gamma_inc(2, 5)
Expected:
    0.0404276819945128
Got:
    gamma(2, 5)

Consider also the following:

sage: gamma_inc(2, 5.)
0.0404276819945128
sage: gamma(2, 5) 
1

comment:9 in reply to: ↑ 8 Changed 11 years ago by burcin

Replying to zimmerma:

Consider also the following:

sage: gamma_inc(2, 5.)
0.0404276819945128
sage: gamma(2, 5) 
1

We should write a python function wrapping gamma_inc and gamma, similar to the provided for psi() in #6961.

The fact that gamma() accepts two arguments at the moment is a bug that I introduced in #7490. Looking at the code, it also doesn't handle the prec parameter correctly. I'll upload a patch with a wrapper gamma() function and a fix for the prec issue.

Replying to fstan:

Integration looks more complicated.

Integration cannot be handled with a custom method in symbolic functions. The patches attached to #6465 should make this easier.

comment:10 Changed 11 years ago by zimmerma

Flavia, please can you fix the rings/complex_number.pyx issue?

Changed 11 years ago by fstan

Added derivative method.

Changed 11 years ago by fstan

Fixed tests from complex_number.pyx

comment:11 Changed 11 years ago by fstan

I've uploaded new patches which should fix the docs and the derivative.

Greetings,
Flavia

Changed 11 years ago by burcin

rebased to 4.3.3.alpha1

Changed 11 years ago by burcin

rebased to 4.3.3.alpha1

Changed 11 years ago by burcin

wrapper for gamma and incomplete gamma

Changed 11 years ago by burcin

wrapper for gamma and incomplete gamma

comment:12 Changed 11 years ago by burcin

  • Status changed from needs_work to needs_review

attachment:trac_7748-gamma_wrapper.2.patch adds a new wrapper function to call gamma or incomplete gamma depending on the number of arguments. It also corrects the way prec parameter to gamma was handled.

Flavia's patches needed to be rebased to 4.3.3.alpha1 since the changes to random_tests.py didn't apply anymore.

The patches to be applied are (in this order):

I'm changing this to needs review. :)

comment:13 Changed 11 years ago by zimmerma

  • Status changed from needs_review to needs_work
  • Work issues set to new test failure

a partial review against 4.3.3:

sage -t  "devel/sage-main/sage/schemes/elliptic_curves/ell_rational_field.py"
...
    sage: E.Lambda(1.4+0.5*I, 50)
      File "/usr/local/sage-4.3.3/sage/local/lib/python/site-packages/sage/schemes/elliptic_curves/ell_rational_field.py", line 3088, in Lambda
        Gamma_inc = transcendental.gamma_inc
    AttributeError: 'module' object has no attribute 'gamma_inc'

This test was ok with vanilla 4.3.3.

comment:14 Changed 11 years ago by zimmerma

apart from the above failure, all tests pass with sage 4.3.3 (apart from the Fedora 12 Segfaults).

Changed 11 years ago by burcin

fix new doctest failure

comment:15 Changed 11 years ago by burcin

  • Reviewers set to Paul Zimmermann
  • Status changed from needs_work to needs_review

I added a fix for the doctest failure in comment:13 to my patch, attachment:trac_7748-gamma_wrapper.3.patch.

The patches to be applied are (in this order):

I am OK with Flavia's changes. If someone can review my patch this can still get in 4.3.4. :)

comment:16 Changed 11 years ago by zimmerma

  • Authors changed from Flavia Stan to Flavia Stan, Burcin Erocal
  • Status changed from needs_review to positive_review

Everything works now. Great work!

Paul

comment:17 follow-up: Changed 11 years ago by burcin

  • Cc mvngu added; burcin removed
  • Work issues new test failure deleted

Hi Minh,

What is holding up this ticket? Note that it has a positive review and the patches to be applied are listed in comment:15.

Cheers,
Burcin

comment:18 in reply to: ↑ 17 Changed 11 years ago by mvngu

Replying to burcin:

What is holding up this ticket?

I can't merge this ticket. We are now in feature freeze.

comment:19 Changed 11 years ago by jhpalmieri

  • Merged in set to sage-4.4.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed

Merged in 4.4.alpha0:

  • trac_7748-exp_integral_ver2.4.3.3.alpha1.patch
  • trac_7748-incomplete_gamma_ver2.4.3.3.alpha1.patch
  • trac_7748-gamma_wrapper.3.patch
Note: See TracTickets for help on using tickets.