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:  sage8.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: 20170729' 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+1k)
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
 Keywords FriCAS added
comment:2 Changed 3 years ago by
comment:3 Changed 3 years ago by
comment:4 Changed 3 years ago by
 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
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
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
 Cc SimonKing added
OK, I found it:
commit 5082c96e79a15b34779a0c9b0b43d652cdade31c Author: Simon King <simon.king@unijena.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 asciiart in interfaces, and this is a default implementation.
comment:8 Changed 3 years ago by
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
 Branch set to u/mantepse/fricas_output_and_sage_conversion_bug
comment:10 Changed 3 years ago by
 Commit set to 20f1d4b42bfc840406e0db132a8970dbf561ff61
 Status changed from new to needs_review
New commits:
20f1d4b  do not strip output in default InterfaceElement._repr_

comment:11 followup: ↓ 13 Changed 3 years ago by
I would suggest to use rstrip instead of strip.
comment:12 Changed 3 years ago by
 Commit changed from 20f1d4b42bfc840406e0db132a8970dbf561ff61 to 5e72d438cbce766972d66af7516b851c82a62ca7
Branch pushed to git repo; I updated commit sha1. New commits:
5e72d43  add a test for the weird quotation mark issue

comment:13 in reply to: ↑ 11 Changed 3 years ago by
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
 Commit changed from 5e72d438cbce766972d66af7516b851c82a62ca7 to 4520a6e734c35ce816bfcc0e7e8b6c5b0b3f6c7c
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

comment:15 Changed 3 years ago by
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
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
Check that #23782 is fixed:
should be
Check that :trac:`23782` is fixed::
comment:18 followup: ↓ 20 Changed 3 years ago by
note also that "atan" will be converted to "arctan" after #22525
comment:19 Changed 3 years ago by
 Commit changed from 4520a6e734c35ce816bfcc0e7e8b6c5b0b3f6c7c to b0ab79f9b3da0cd5862a9438e6f0d34b7947c4d0
Branch pushed to git repo; I updated commit sha1. New commits:
b0ab79f  fix reference to trac

comment:20 in reply to: ↑ 18 Changed 3 years ago by
comment:21 Changed 3 years ago by
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
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
 Dependencies set to #22525
I'll undo the modified line in integral.py after recompiling.
comment:24 Changed 3 years ago by
 Commit changed from b0ab79f9b3da0cd5862a9438e6f0d34b7947c4d0 to c4a44f633790effdff1d4123e8a7a7c3155e9c9e
Branch pushed to git repo; I updated commit sha1. New commits:
c4a44f6  undo edit of test output  this is fixed by #22525 anyway

comment:25 Changed 3 years ago by
I think this is ready now.
comment:26 Changed 3 years ago by
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
 Commit changed from c4a44f633790effdff1d4123e8a7a7c3155e9c9e to 2521f3a9556ea763d418df9e04b36e84b6137cb3
Branch pushed to git repo; I updated commit sha1. New commits:
2521f3a  Merge branch 'develop' into t/23782/fricas_output_and_sage_conversion_bug

comment:28 Changed 3 years ago by
 Cc chapoton added
comment:30 Changed 3 years ago by
 Reviewers set to Ralf Stephan
 Status changed from needs_review to needs_work
Using optional=fricas I get
sage t warnlong 40.1 src/sage/interfaces/fricas.py # 3 doctests failed
comment:31 Changed 3 years ago by
I was told to use
sage t optional fricas,sage src/sage/interfaces/fricas.py
comment:32 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:33 Changed 3 years ago by
 Status changed from needs_review to positive_review
Alright, interesting, thanks. It passes now and looks good.
comment:34 Changed 3 years ago by
Thank you!
comment:35 Changed 3 years ago by
 Branch changed from u/mantepse/fricas_output_and_sage_conversion_bug to 2521f3a9556ea763d418df9e04b36e84b6137cb3
 Resolution set to fixed
 Status changed from positive_review to closed
This seems to work after #21377