Opened 3 years ago

Closed 3 years ago

#23782 closed defect (fixed)

fricas output and sage conversion bug

Reported by: mantepse Owned by:
Priority: major Milestone: sage-8.1
Component: interfaces: optional Keywords: FriCAS
Cc: SimonKing, chapoton, rws Merged in:
Authors: Martin Rubey Reviewers: Ralf Stephan
Report Upstream: N/A Work issues:
Branch: 2521f3a (Commits) Commit: 2521f3a9556ea763d418df9e04b36e84b6137cb3
Dependencies: #22525 Stopgaps:

Description

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.

Change History (35)

comment:1 Changed 3 years ago by chapoton

  • Keywords FriCAS added

comment:2 Changed 3 years ago by chapoton

This seems to work after #21377

comment:3 Changed 3 years ago by mantepse

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.

comment:4 Changed 3 years ago by mantepse

  • Type changed from PLEASE CHANGE to defect

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!

comment:5 Changed 3 years ago by mantepse

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.

comment:6 Changed 3 years ago by mantepse

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'

comment:7 Changed 3 years ago by mantepse

  • Cc SimonKing added

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.

comment:8 Changed 3 years ago by mantepse

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.

comment:9 Changed 3 years ago by mantepse

  • Branch set to u/mantepse/fricas_output_and_sage_conversion_bug

comment:10 Changed 3 years ago by mantepse

  • Authors set to Martin Rubey
  • Commit set to 20f1d4b42bfc840406e0db132a8970dbf561ff61
  • Status changed from new to needs_review

New commits:

20f1d4bdo not strip output in default InterfaceElement._repr_

comment:11 follow-up: Changed 3 years ago by chapoton

I would suggest to use rstrip instead of strip.

comment:12 Changed 3 years ago by git

  • Commit changed from 20f1d4b42bfc840406e0db132a8970dbf561ff61 to 5e72d438cbce766972d66af7516b851c82a62ca7

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

5e72d43add a test for the weird quotation mark issue

comment:13 in reply to: ↑ 11 Changed 3 years ago by mantepse

Replying to chapoton:

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?

comment:14 Changed 3 years ago by git

  • Commit changed from 5e72d438cbce766972d66af7516b851c82a62ca7 to 4520a6e734c35ce816bfcc0e7e8b6c5b0b3f6c7c

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

4520a6euse rstrip in _repr_ (instead of original strip and instead of nothing), adapt tests

comment:15 Changed 3 years ago by mantepse

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

comment:16 Changed 3 years ago by mantepse

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!

comment:17 Changed 3 years ago by chapoton

Check that #23782 is fixed:

should be

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

comment:18 follow-up: Changed 3 years ago by chapoton

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

comment:19 Changed 3 years ago by git

  • Commit changed from 4520a6e734c35ce816bfcc0e7e8b6c5b0b3f6c7c to b0ab79f9b3da0cd5862a9438e6f0d34b7947c4d0

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

b0ab79ffix reference to trac

comment:20 in reply to: ↑ 18 Changed 3 years ago by mantepse

Replying to chapoton:

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?

comment:21 Changed 3 years ago by chapoton

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.

comment:22 Changed 3 years ago by mantepse

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

comment:23 Changed 3 years ago by mantepse

  • Dependencies set to #22525

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

comment:24 Changed 3 years ago by git

  • Commit changed from b0ab79f9b3da0cd5862a9438e6f0d34b7947c4d0 to c4a44f633790effdff1d4123e8a7a7c3155e9c9e

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

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

comment:25 Changed 3 years ago by mantepse

I think this is ready now.

comment:26 Changed 3 years ago by mantepse

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

comment:27 Changed 3 years ago by git

  • Commit changed from c4a44f633790effdff1d4123e8a7a7c3155e9c9e to 2521f3a9556ea763d418df9e04b36e84b6137cb3

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

2521f3aMerge branch 'develop' into t/23782/fricas_output_and_sage_conversion_bug

comment:28 Changed 3 years ago by mantepse

  • Cc chapoton added

comment:29 Changed 3 years ago by mantepse

  • Cc rws added

ping?

comment:30 Changed 3 years ago by rws

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

Using --optional=fricas I get

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

comment:31 Changed 3 years ago by mantepse

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

Last edited 3 years ago by mantepse (previous) (diff)

comment:32 Changed 3 years ago by mantepse

  • Status changed from needs_work to needs_review

comment:33 Changed 3 years ago by rws

  • Status changed from needs_review to positive_review

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

comment:34 Changed 3 years ago by mantepse

Thank you!

comment:35 Changed 3 years ago by vbraun

  • Branch changed from u/mantepse/fricas_output_and_sage_conversion_bug to 2521f3a9556ea763d418df9e04b36e84b6137cb3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.