conversion of I to fricas is wrong

mantepse commented 6 years ago

This code in this ticket corrects the conversion of tThe imaginary unit (as a symbolic expression) between sagemath and FriCAS. In FriCAS, it's representation is "%i", not "I". Before the ticket, we had:

sage: fricas(I)^2
 2
I
sage: fricas("%i")^2
- 1

After this ticket, also more complex stuff works:

sage: integrate(sin(x)*exp(I*x), x, -pi, 0, algorithm="fricas")
1/2*I*pi

The conversion from SageMath to FriCAS is done in complete analogy to maxima. This may not be ideal, but improving that if necessary should be done in a different ticket.

The conversion from FriCAS to SageMath is a straightforward extension in the interface code. After this ticket, complex symbolic expressions and polynomials are translated to elements of the Symbolic Ring.

CC: @rwst

Component: interfaces

Keywords: FriCAS

Author: Martin Rubey

Branch/Commit: d076fda

Reviewer: Vincent Delecroix

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

rwst commented 6 years ago

comment:1

Fricas is accessed through an interface, see sage/interfaces/fricas.py. Converting a Sage object to an interface object is done by implementing a _interface_ member function for the object. Note however that I is not a Constant object but a number field element (embedded in a symbolic expression, try type(I.pyobject())).

mantepse commented 6 years ago

comment:2

Should I do it similar to https://github.com/sagemath/sage-prod/files/10646990/trac_7557-maxima_complex_number_conversion.2.patch.gz ?

rwst commented 6 years ago

comment:3

I is not a complex number. This is just to show where to put it:

diff --git a/src/sage/rings/number_field/number_field_element.pyx b/src/sage/rings/number_field/number_field_element.pyx
index c9a6ef42bd..c4986630f1 100644
--- a/src/sage/rings/number_field/number_field_element.pyx
+++ b/src/sage/rings/number_field/number_field_element.pyx
@@ -730,6 +730,11 @@ cdef class NumberFieldElement(FieldElement):
         """
         return repr(self.__pari__(name=name))

+    def _interface_init_(self, I):
+        if repr(self) == 'I':
+            return "%i"
+        raise NotImplementedError
+
     def __getitem__(self, n):
         """
         Return the n-th coefficient of this number field element, written

It works but may not be the correct way to do it.

videlec commented 6 years ago

comment:4

Indeed it is not

sage: K.<I> = NumberField(x^3 - 2)

And I assume that it should be _fricas_init_ instead of _interface_init_.

videlec commented 6 years ago

comment:5

Actually the conversion for maxima is implemented in sage/rings/number_field/number_field_element_quadratic.pyx as

    def _maxima_init_(self, I=None):
        """
        EXAMPLES::

            sage: K.<a> = QuadraticField(-1)
            sage: f = 1 + a
            sage: f._maxima_init_()
            '1+%i*1'
        """
        a = self.parent().gen()
        if a**2 == -1:
            x0, x1 = self
            return str(x0) + "+" + "%i*" + str(x1)
        else:
            NumberFieldElement_absolute._maxima_init_(self, I)

which is not bullet proof at all with respect to embedding

sage: K.<J> = QuadraticField(-1, embedding=CC(0,-1))
sage: J._maxima_init_()
'0+%i*1'

mantepse commented 6 years ago

Branch: u/mantepse/conversion_of_i_to_fricas_is_wrong

mantepse commented 6 years ago

Commit: c904442

mantepse commented 6 years ago

comment:7

This is a first attempt, however, locally testing fails currently.

New commits:

c904442 enable conversion of I to and from fricas

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

Changed commit from c904442 to 277b58d

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

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

277b58d fix import

mantepse commented 6 years ago

Author: Martin Rubey

mantepse commented 6 years ago

Description changed:

--- 
+++ 
@@ -1,4 +1,4 @@
-The imaginary unit (as a symbolic expression) in FriCAS is "%i", not "I":
+This code in this ticket corrects the conversion of tThe imaginary unit (as a symbolic expression) between sagemath and FriCAS.  In FriCAS, it's representation is "%i", not "I".  Before the ticket, we had:

sage: fricas(I)^2 @@ -7,4 +7,14 @@ sage: fricas("%i")^2

1

-However, I was unable to find out where to change that.
+
+After this ticket, also more complex stuff works:
+
+```
+sage: integrate(sin(x)*exp(I*x), x, -pi, 0, algorithm="fricas")
+1/2*I*pi
+```
+
+The conversion from [SageMath](../wiki/SageMath) to FriCAS is done in complete analogy to maxima.  This may not be ideal, but improving that if necessary should be done in a different ticket.
+
+The conversion from FriCAS to [SageMath](../wiki/SageMath) is a straightforward extension in the interface code.  After this ticket, complex symbolic expressions and polynomials are translated to elements of the `Symbolic Ring`.

videlec commented 6 years ago

comment:11

Since _fricas_init_ is the exact same as _maxima_init_ you should rather create a unique method for both of them (and add aliases).

As I mentioned in [comment:5] the test a**2 == -1 is not enough. You should also consider the embedding. There is an attribute standard_embedding so that the test could become

if a**2 == -1 and a.standard_embedding:
    ...

You should also add a doctest for the case when the embedding is the non standard one.

videlec commented 6 years ago

Reviewer: Vincent Delecroix

mantepse commented 6 years ago

comment:12

Replying to @videlec:

Since _fricas_init_ is the exact same as _maxima_init_ you should rather create a unique method for both of them (and add aliases).

How do I implement the distinction NumberFieldElement_absolute._fricas_init_(self, I) vs. NumberFieldElement_absolute._maxima_init_(self, I). Some super magic?

As I mentioned in [comment:5] the test a**2 == -1 is not enough. You should also consider the embedding. There is an attribute standard_embedding so that the test could become
if a**2 == -1 and a.standard_embedding:
    ...

You mean self.standard_embedding. right? Doing so, I get

sage: K.<J> = QuadraticField(-1, embedding=CC(0,-1))
sage: maxima(J)
...
TypeError: _maxima_init_() takes no arguments (1 given)

which is probably suboptimal.

You should also add a doctest for the case when the embedding is the non standard one.

Could you please propose one?

videlec commented 6 years ago

comment:13

Replying to @mantepse:

Replying to @videlec:

Since _fricas_init_ is the exact same as _maxima_init_ you should rather create a unique method for both of them (and add aliases).

How do I implement the distinction NumberFieldElement_absolute._fricas_init_(self, I) vs. NumberFieldElement_absolute._maxima_init_(self, I). Some super magic?

The super magic is called alias

class A:
    def f(self): return 3

    g = f

As I mentioned in [comment:5] the test a**2 == -1 is not enough. You should also consider the embedding. There is an attribute standard_embedding so that the test could become
if a**2 == -1 and a.standard_embedding:
    ...
You mean self.standard_embedding. right? Doing so, I get
sage: K.<J> = QuadraticField(-1, embedding=CC(0,-1))
sage: maxima(J)
...
TypeError: _maxima_init_() takes no arguments (1 given)
which is probably suboptimal.

Indeed, it should be a ValueError or NotImplementedError.

You should also add a doctest for the case when the embedding is the non standard one.

Could you please propose one?

sage: K.<J> = QuadraticField(-1, embedding=-QQbar.gen())
sage: maxima(J)
sage: fricas(J)

mantepse commented 6 years ago

comment:14

Replying to @videlec:

Replying to @mantepse:

Replying to @videlec:

Since _fricas_init_ is the exact same as _maxima_init_ you should rather create a unique method for both of them (and add aliases).

How do I implement the distinction NumberFieldElement_absolute._fricas_init_(self, I) vs. NumberFieldElement_absolute._maxima_init_(self, I). Some super magic?

The super magic is called alias
class A:
    def f(self): return 3

    g = f

I don't get it. We have:

class A:
    def f(self): return superclass.f()
    def g(self): return superclass.g()

Is this really the same as

class A:
    def f(self): return superclass.f()
    g = f

?

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

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

e3c5f56 fix and factor NumberFieldElement_quadratic._maxima_init_ and _fricas_init_

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

Changed commit from 277b58d to e3c5f56

videlec commented 6 years ago

comment:17

The error message is not quite accurate. The discriminant is the first reason why the conversion might fail.

sage: K.<sqrt2> = QuadraticField(2, embedding=AA(2))
sage: maxima(sqrt2) # failing but not for a bad ebedding reason

Perhaps "maxima/fricas conversions only implemented for quadratic field with discrimnant -1 and standard embedding" (though a bit long).

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

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

d076fda more precise error message

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

Changed commit from e3c5f56 to d076fda

vbraun commented 6 years ago

Changed branch from u/mantepse/conversion_of_i_to_fricas_is_wrong to d076fda

sagemath / sage

conversion of I to fricas is wrong #25174