fricas output and sage conversion bug

[mantepse]

In 'SageMath version 8.1.beta0, Release Date: 2017-07-29' I have the following behaviour, (at least with FriCAS 1.2.5)

def elementary(k, j):
    """
    sage: f = fricas.guessRat([elementary(5,j) for j in range(15)])[0]; f
    10      9      8      7       6      5       4      3       2
    3n   - 25n  + 50n  + 62n  - 229n  - 25n  + 320n  - 12n  - 144n
    ---------------------------------------------------------------
                                 11520

    sage: print fricas.get(f._name)
      10      9      8      7       6      5       4      3       2
    3n   - 25n  + 50n  + 62n  - 229n  - 25n  + 320n  - 12n  - 144n
    ---------------------------------------------------------------
                                 11520

    sage: f.sage()
    NotImplementedError: The translation of the FriCAS Expression "(3*n^10+-25*n^9+50*n^8+62*n^7+-229*n^6+-25*n^5+320*n^4+-12*n^3+-144*n^2)/11520 to sage is not yet implemented.
    """
    return stirling_number1(j+1, j+1-k)

Note the bad indentation in the second line and the weird quotation mark in the third.

Fixing #22525 would probably fix this, too.

Depends on #22525

CC: @simon-king-jena @fchapoton @rwst

Component: interfaces: optional

Keywords: FriCAS

Author: Martin Rubey

Branch/Commit: 2521f3a

Reviewer: Ralf Stephan

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

[fchapoton]

Changed keywords from none to FriCAS

[fchapoton]

comment:2

This seems to work after #21377

[mantepse]

comment:3

After #21377 I still get the bad indentation, but the error is gone. So I vote to close...

I'll put the remark concerning the bad indentation on #21377.

[mantepse]

comment:4

I just realised that I had a sage 7.5.beta3 lying around:

the display bug was introduced after 7.5.beta3 - the bug with the quotation mark existed already in 7.5.beta3!

[mantepse]

comment:5

Moreover, there is no change in the code of fricas.py between sage 7.5.beta3 and 8.0.beta12, which is the first version I have on my computer where the bug appears.

I have no idea what could have changed - I'm afraid I need to bisect.

[mantepse]

comment:6

sage:  fricas('(1 + sqrt(2))^5')

+-+
29\|2  + 41
sage: a= fricas('(1 + sqrt(2))^5')
sage: print a
+-+
29\|2  + 41
sage: print fricas.get(a._name)
   +-+
29\|2  + 41
sage: repr(a)
'+-+\r\n29\\|2  + 41'
sage: fricas.get(a._name)
'   +-+\r\n29\\|2  + 41'

[mantepse]

comment:7

OK, I found it:

commit 5082c96e79a15b34779a0c9b0b43d652cdade31c
Author: Simon King <simon.king@uni-jena.de>
Date:   Tue Mar 14 14:53:36 2017 +0100

    Use single underscore _repr_ in interfaces, add more documentation concerning eval and get

introduced this change in #22501:

+    def _repr_(self):
+        """
+        Default implementation of a helper method for string representation.
...
+        """
+        P = self.parent()
+        try:
+            if self._get_using_file:
+                return P.get_using_file(self._name).strip()
+        except AttributeError:
+            return self.parent().get(self._name).strip()

I think the strip should go away. The other solution is to override it in fricas.py, but I really cannot think of a situation where we really want the strip. Output will often be ascii-art in interfaces, and this is a default implementation.

[mantepse]

comment:8

I want to add that I commented on #22501 that fricas tests would be passing. This means that I made a mistake then, so the blame is on me.

[mantepse]

Branch: u/mantepse/fricas_output_and_sage_conversion_bug

[mantepse]

Commit: 20f1d4b

[mantepse]

Author: Martin Rubey

[mantepse]

New commits:

20f1d4b

do not strip output in default InterfaceElement._repr_

[fchapoton]

comment:11

I would suggest to use rstrip instead of strip.

[sagetrac-git]

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

5e72d43

add a test for the weird quotation mark issue

[sagetrac-git]

Changed commit from 20f1d4b to 5e72d43

[mantepse]

comment:13

Replying to @fchapoton:

I would suggest to use rstrip instead of strip.

I agree that this is a sensible possibility, but I'm not completely sure.

Can you think of any surprising results with rstrip?

[sagetrac-git]

Changed commit from 5e72d43 to 4520a6e

[sagetrac-git]

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

4520a6e

use rstrip in `_repr_` (instead of original strip and instead of nothing), adapt tests

[mantepse]

comment:15

I tested fricas, lie and gap3. Unfortunately, gap3 is failing, but I do not know yet whether the failures are related to this ticket.

[mantepse]

comment:16

I get the same failures in all other versions of sage I have lying around... So I think that this is really ready to go. Please review!

[fchapoton]

comment:17

Check that #23782 is fixed:

should be

Check that :trac:`23782` is fixed::

[fchapoton]

comment:18

note also that "atan" will be converted to "arctan" after #22525

[sagetrac-git]

Changed commit from 4520a6e to b0ab79f

[sagetrac-git]

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

b0ab79f

fix reference to trac

[mantepse]

comment:20

Replying to @fchapoton:

note also that "atan" will be converted to "arctan" after #22525

yes, I just looked at it, and have a branch that only adds some testcases - I would set it to positive review there and make this one a dependency. Or would you prefer it the other way round?

[fchapoton]

comment:21

The branch here is slightly more problematic, because you change the behaviour of all the interfaces and this needs serious checking. The other one should be easier and go first, imho.

[mantepse]

comment:22

I agree. So let's do the other one first. I have problems with checking out right now, though.

[mantepse]

comment:23

I'll undo the modified line in integral.py after recompiling.

[mantepse]

Dependencies: #22525

[sagetrac-git]

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

c4a44f6

undo edit of test output - this is fixed by #22525 anyway

[sagetrac-git]

Changed commit from b0ab79f to c4a44f6

[mantepse]

comment:25

I think this is ready now.

[mantepse]

comment:26

There will be a merge conflict - I guess it's easiest to fix it when #22525 is merged.

[sagetrac-git]

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

2521f3a

Merge branch 'develop' into t/23782/fricas_output_and_sage_conversion_bug

[sagetrac-git]

Changed commit from c4a44f6 to 2521f3a

[mantepse]

comment:29

ping?

[rwst]

comment:30

Using --optional=fricas I get

sage -t --warn-long 40.1 src/sage/interfaces/fricas.py # 3 doctests failed

[rwst]

Reviewer: Ralf Stephan

[mantepse]

comment:31

I was told to use
sage -t --optional fricas,sage src/sage/interfaces/fricas.py

[rwst]

comment:33

Alright, interesting, thanks. It passes now and looks good.

[mantepse]

comment:34

Thank you!

[vbraun]

Changed branch from u/mantepse/fricas_output_and_sage_conversion_bug to 2521f3a