sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.45k stars 482 forks source link

CIF is missing many functions #17285

Closed jdemeyer closed 9 years ago

jdemeyer commented 10 years ago
sage: CIF(cos(3/2))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-31-4eae9038f0b3> in <module>()
----> 1 CIF(cos(Integer(3)/Integer(2)))

/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/rings/complex_interval_field.pyc in __call__(self, x, im)
    378 
    379             try:
--> 380                 return x._complex_mpfi_( self )
    381             except AttributeError:
    382                 pass

/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/symbolic/expression.so in sage.symbolic.expression.Expression._complex_mpfi_ (build/cythonized/sage/symbolic/expression.cpp:8043)()

The reason that this fails is:

sage: CIF(3/2).cos()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-13-96dae572cb36> in <module>()
----> 1 CIF(Integer(3)/Integer(2)).cos()

/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/structure/element.so in sage.structure.element.Element.__getattr__ (build/cythonized/sage/structure/element.c:4068)()

/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/structure/misc.so in sage.structure.misc.getattr_from_other_class (build/cythonized/sage/structure/misc.c:1631)()

AttributeError: 'sage.rings.complex_interval.ComplexIntervalFieldElement' object has no attribute 'cos'

This problem was also encoutered on this ask thread.

follow up: #18135 and #18136

Depends on #17130

CC: @sagetrac-tmonteil @videlec

Component: basic arithmetic

Author: Vincent Delecroix

Branch/Commit: afccfe6

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/17285

jdemeyer commented 10 years ago

Description changed:

--- 
+++ 
@@ -16,13 +16,18 @@
 /usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/symbolic/expression.so in sage.symbolic.expression.Expression._complex_mpfi_ (build/cythonized/sage/symbolic/expression.cpp:8043)()

-Similar things do work: +The reason that this fails is:

-sage: RIF(cos(3/2))
-0.0707372016677030?
-sage: CC(cos(3/2))
-0.0707372016677029
-sage: CIF(cos(3))
--0.98999249660044542?
+sage: CIF(3/2).cos()
+---------------------------------------------------------------------------
+AttributeError                            Traceback (most recent call last)
+<ipython-input-13-96dae572cb36> in <module>()
+----> 1 CIF(Integer(3)/Integer(2)).cos()
+
+/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/structure/element.so in sage.structure.element.Element.__getattr__ (build/cythonized/sage/structure/element.c:4068)()
+
+/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/structure/misc.so in sage.structure.misc.getattr_from_other_class (build/cythonized/sage/structure/misc.c:1631)()
+
+AttributeError: 'sage.rings.complex_interval.ComplexIntervalFieldElement' object has no attribute 'cos'
edd8e884-f507-429a-b577-5d554626c0fe commented 10 years ago

Description changed:

--- 
+++ 
@@ -31,3 +31,6 @@

 AttributeError: 'sage.rings.complex_interval.ComplexIntervalFieldElement' object has no attribute 'cos'

+ +This problem was also encoutered on this ask thread. +

edd8e884-f507-429a-b577-5d554626c0fe commented 10 years ago
comment:2

Are you working on this ?

Is a solution involving the use of the (already defined) .exp() method acceptable on the short term (it may not lead to the thinest possible intervals), or do you think about some more accurate way of writing those ?

jdemeyer commented 10 years ago
comment:3

Replying to @sagetrac-tmonteil:

Are you working on this ?

No.

Is a solution involving the use of the (already defined) .exp() method acceptable on the short term (it may not lead to the thinest possible intervals), or do you think about some more accurate way of writing those ?

I think it's acceptable to rely on exp() but instead, I would rather use the structure of the exp() method: completely reduce the computation to a computation in RIF.

fredrik-johansson commented 10 years ago
comment:4

For best accuracy, one should use cos(a+bi) = cos(a)*cosh(b) + sin(a)*sinh(b)*i and sin(a+bi) = sin(a)*cosh(b) + cos(a)*sinh(b)*i (if I got those formulas right).

There's a mpfi_sin_cos but it looks like there's no mpfi_sinh_cosh yet.

videlec commented 9 years ago
comment:5

This is also responsible for

sage: a = RLF(cos(2/3))
sage: b = CLF(QQbar(-2).sqrt())
sage: a + b
<repr(<sage.rings.real_lazy.LazyBinop at 0x7fb7ffc28680>) failed: TypeError: unable to simplify to a complex interval approximation>
videlec commented 9 years ago

Author: Vincent Delecroix

videlec commented 9 years ago

New commits:

dd4cf4eTrac 17285: CIF trigonometric functions
videlec commented 9 years ago

Commit: dd4cf4e

videlec commented 9 years ago

Branch: u/vdelecroix/17285

videlec commented 9 years ago
comment:7

This is not the fastest and safest way possible (the obtained interval is sometimes too large). But at least, the given implementation has the feature that trigonometric functions preserve real and imaginary axes...

Vincent

videlec commented 9 years ago
comment:8

Note: the link to the ask question is dead!

jdemeyer commented 9 years ago
comment:9

I don't like the added complexity of having 3 branches in every function. Why not just use the generic case everywhere?

videlec commented 9 years ago
comment:10

Replying to @jdemeyer:

I don't like the added complexity of having 3 branches in every function. Why not just use the generic case everywhere?

The main reason is to avoid numerical noise (I want the cosine of a real number to be a real number). You have

sage: a = CIF(2,0)
sage: I = CIF.gen()
sage: ((I*a).exp() + (-I*a).exp()) / 2
-0.4161468365471424? + 0.?e-16*I

It should be fine to remove the imaginary branch, but then it would be less precise

sage: a = CIF(0,2)
sage: b1 = CIF(0, a.imag().cosh())
sage: b1.diameter()
1.33393183261575e-16
sage: b2 = ((I*a).exp() + (-I*a).exp()) / 2
sage: b2.diameter()
2.36079803558624e-16

Vincent

jdemeyer commented 9 years ago
comment:11

For the sine and cosine, you can use the formula at the end of http://en.wikipedia.org/wiki/Trigonometric_functions#Relationship_to_exponential_function_and_complex_numbers which computes the real and imaginary part separately. Assuming that sinh(0) = 0 exactly, this formula will not introduce a fake imaginary part.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from dd4cf4e to 3f8cd41

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

3f8cd41Trac 17285: CIF trigonometric functions
videlec commented 9 years ago
comment:13

New implementation using other trigonometric identities (and much faster).

For tan and tanh this is certainly not the best possible.

Vincent

jdemeyer commented 9 years ago

Reviewer: Jeroen Demeyer

jdemeyer commented 9 years ago
comment:14

You're not actually supposed to do this:

from sage.ext.interrupt cimport sig_on, sig_off

The official way is still include "sage/ext/interrupt.pxi" (and this will even become mandatory after #18027)

jdemeyer commented 9 years ago
comment:15

I like the new code. However, a few comments:

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

afccfe6Trac 17285: signal handling + doc
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 3f8cd41 to afccfe6

videlec commented 9 years ago
comment:19

Thanks for the review.

Just one related question. With the current code, if a KeyboardInterrupt happens in between the sig_on/sig_off block, then the code mpfi_clear(tmp) will never get called and hence the memory allocated to tmp will not be freed. Is there a clean solution for that?

jdemeyer commented 9 years ago
comment:20

Replying to @videlec:

Is there a clean solution for that?

try:/finally:.

videlec commented 9 years ago

Description changed:

--- 
+++ 
@@ -34,3 +34,4 @@

 This problem was also encoutered [on this ask thread](http://ask.sagemath.org/question/10115/problem-with-conjugate_transpose-of-a-symbolic-matrix/?answer=14912#post-id-14912).

+follow up: #18135 and #18136
vbraun commented 9 years ago

Changed branch from u/vdelecroix/17285 to afccfe6