Opened 5 years ago

Closed 5 years ago

# improve FriCAS interface

Reported by: Owned by: mantepse major sage-7.4 interfaces FriCAS bpage, hemmecke, dkrenn, dimpase Martin Rubey Bill Page, Emmanuel Charpentier N/A e281742 e281742685eb891ff7883bd6e91b16e8c31b92d6 #21209

### Description

The FriCAS interface is currently very rudimentary. In particular, converting the results of a computation into sage types is available only in very few special cases.

### comment:1 Changed 5 years ago by mantepse

1) implement a method that (reliably!) yields a tuple (type, 1d algebra output, 2d algebra output, error message, etc) as strings. 1d algebra output should come in "unparsed inputform" as one very long string, I'd say. 2d algebra output should be empty if 1d output is available.

It should be able to deal with very long lines, too, possibly be using file io. This involves use of ioHook and some regexps.

2) implement a method that translates fricas output into sage types.

I think this could work as follows: we want to map fricas types to sage constructors, possibly recursively. Recall that what's really sent to fricas is something like "sage23 := [x^n/y^n for n in 1..3]", so that sage can access the result using the variable "sage23". This string is referred to as self._name

Now what I propose is a method which takes a parsed type, e.g., "Integer" or ("List", "Integer") or ("UnivariatePolynomial", "x", ("Fraction", "Integer"))

def to_sage(type):
if type == "Integer":
elif type == "String":
...
elif isinstance(type, tuple):
if type[0] == "List":
elif type[0] == "Fraction":
....


and so on.

So, if the type is ("List" ("Fraction" ("UnivariatePolynomial" "x" "Integer"))), this method would call

    to_sage_list(("Fraction" ("UnivariatePolynomial" "x" "Integer"))


which might be something like

def to_sage_list(self, type):
n = fricas.eval("#" + self._name).to_sage_integer()
return [fricas.eval(self._name + ".%n").to_sage(type) for n in range(1,n+1)]


### comment:2 Changed 5 years ago by mantepse

• Type changed from PLEASE CHANGE to enhancement

### comment:3 Changed 5 years ago by mantepse

• Branch set to /u/mantepse/21231
• Commit set to 5b7e6ab305a5d4967bd658e9ce991683a4ebdac9

New commits:

 ​5fa5c04 Merge branch 'u/mantepse/generating_function_in_findstat_interface' of git://trac.sagemath.org/sage into develop ​4b555c5 Merge branch 'develop' of git://github.com/sagemath/sage into develop ​17f7dbe Merge branch 'u/dkrenn/16137' of git://trac.sagemath.org/sage into develop ​08494a2 Merge branch 'u/vdelecroix/16137' of git://trac.sagemath.org/sage into develop ​f077211 Merge branch 'develop' of git://github.com/sagemath/sage into develop ​5b7e6ab initial version of new fricas interface

### comment:4 Changed 5 years ago by mantepse

This is very beta, but I'd be very happy to receive some comments.

### comment:5 Changed 5 years ago by mantepse

• Authors set to Martin Rubey
• Commit changed from 5b7e6ab305a5d4967bd658e9ce991683a4ebdac9 to d8a26e149d4f182817c3664af11c38b8f55488c4

New commits:

 ​d8a26e1 Merge remote-tracking branch 'origin/develop' into fricas-interface

### comment:6 follow-up: ↓ 7 Changed 5 years ago by vbraun

• Branch changed from /u/mantepse/21231 to u/mantepse/21231
• Commit changed from d8a26e149d4f182817c3664af11c38b8f55488c4 to b2ee8cca6b2ce9320f31b3c457b86f048dd66a54

Branch name doesn't have leading slash

New commits:

 ​b2ee8cc make it build

### comment:7 in reply to: ↑ 6 ; follow-up: ↓ 8 Changed 5 years ago by mantepse

Branch name doesn't have leading slash

Howto? I used

git push --set-upstream trac HEAD:u/mantepse/21231


### comment:8 in reply to: ↑ 7 Changed 5 years ago by dimpase

Branch name doesn't have leading slash

Howto? I used

git push --set-upstream trac HEAD:u/mantepse/21231


This is fine, this does not do anything to the webpage contents. You made a typo, this leading slash, while editing the latter.

### comment:9 Changed 5 years ago by dimpase

• Dependencies set to #21209

### comment:10 Changed 5 years ago by mantepse

(it actually doesn't depend on #21209, since it works with any installation of FriCAS but never mind)

### comment:11 Changed 5 years ago by git

• Commit changed from b2ee8cca6b2ce9320f31b3c457b86f048dd66a54 to cbcb94f2cf3b54926be73c4eb842df5da0096d73

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

 ​cbcb94f quick dirty fix for bug reported by Bill Page, add idea for better type handling

### Changed 5 years ago by bpage

Sage worksheet under 7.2

### Changed 5 years ago by bpage

Sage worksheet under 7.4.beta0 (typeset output fails)

### comment:12 Changed 5 years ago by bpage

In Sage Worksheet typeset output or FriCAS object fails with this patch on 7.4.beta0. See attached pdf files.

https://trac.sagemath.org/attachment/ticket/21231/sage-7.2-fricas.pdf (typeset output OK)

https://trac.sagemath.org/attachment/ticket/21231/sage-7.4.beta0-fricas.pdf (typeset output fails)

I am not sure whether or not this example worked under 7.4.beta0 without the patch.

### comment:13 Changed 5 years ago by git

• Commit changed from cbcb94f2cf3b54926be73c4eb842df5da0096d73 to 5b4a95d69c1e43fd29f18a07a2d1b7bb6f904459

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

 ​5b4a95d switch to better type treatment

### comment:14 Changed 5 years ago by git

• Commit changed from 5b4a95d69c1e43fd29f18a07a2d1b7bb6f904459 to 0ab0f924f297669e203031ab9f74677eee7c8361

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

 ​0ab0f92 further improve translation to sage objects, make restart work, fix doctests

### comment:15 Changed 5 years ago by mantepse

I have a question about the _fricas_init_ and _fricas_ methods defined in various files, in particular in ~/sage-develop/src/sage/rings/rational_field.py and in ~/sage-develop/src/sage/rings/real_mpfr.pyx, see below.

The question is: the doctests work without adapting the methods. What are the methods supposed to do?

~/sage-develop/src/sage/rings/rational_field.py

    def _axiom_init_(self):
r"""
Return the axiom/fricas representation of \QQ.

EXAMPLES::

sage: axiom(QQ)    #optional - axiom # indirect doctest
Fraction Integer
sage: fricas(QQ)   #optional - fricas # indirect doctest
Fraction(Integer)

"""
return 'Fraction Integer'

_fricas_init_ = _axiom_init_


in ~/sage-develop/src/sage/rings/real_mpfr.pyx

    def _axiom_(self, axiom):
"""
Return self as a floating point number in Axiom.

EXAMPLES::

sage: R = RealField(100)
sage: R(pi)
3.1415926535897932384626433833
sage: axiom(R(pi))  # optional - axiom # indirect doctest
3.1415926535 8979323846 26433833
sage: fricas(R(pi)) # optional - fricas
3.1415926535_8979323846_26433833

"""
prec = self.parent().prec()

#Set the precision in Axiom
old_prec = axiom('precision(%s)$Float'%prec) res = axiom('%s :: Float'%self.exact_rational()) axiom.eval('precision(%s)$Float'%old_prec)

return res

_fricas_ = _axiom_


### comment:16 Changed 5 years ago by git

• Commit changed from 0ab0f924f297669e203031ab9f74677eee7c8361 to d2d0fb89696d255c0a583815832b2302e63d1215

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

 ​d2d0fb8 fix tab completion

### comment:17 Changed 5 years ago by mantepse

The once missing thing is the error handling. I was expecting that raising an error in eval would be sufficient, as indicated by the patch below. However, if I then do fricas("something stupid"), no error is raised. What am I doing wrong?

--- a/src/sage/interfaces/fricas.py
+++ b/src/sage/interfaces/fricas.py
@@ -385,8 +385,7 @@ class FriCAS(ExtraTabCompletion, Expect):
return "\r\n".join(line[FRICAS_MULTI_LINE_START:] for line in lines)

else:
-            print(output)
-            return
+            raise RuntimeError("FriCAS has not recognized '%s' as a valid operation: %s" % (code, output))

def set(self, var, value):
"""Set the variable var to the given value.


### comment:18 Changed 5 years ago by bpage

Calling fricas("something stupid") ends up in fricas.set which calls expect._eval_line not fricas.eval. This interface stuff is truly twisted. Check especially the inheritance. FriCAS inherits from Expect that inherits Interface. Check __call__ and _create in Interface and go from there.

fricas.py should probably override _eval_line in the same way as axiom.py.

Last edited 5 years ago by bpage (previous) (diff)

### comment:19 Changed 5 years ago by git

• Commit changed from d2d0fb89696d255c0a583815832b2302e63d1215 to 148bf77e2d3504f9d3dbbda038ca4270dd81bd83

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

 ​148bf77 better to ask for forgiveness than permission in __getitem__, some series examples, raise error in eval

### comment:20 follow-up: ↓ 22 Changed 5 years ago by bpage

fricas.py used to inherit some functions from axiom.py. The new version no longer uses any part of the Axiom interface. One of the casualties of this change was _latex_ from PanAxiomElement. The function _latex_ is responsible for producing LaTeX output in typeset mode. The following patch incorporates the _latex_ function from axiom.py and corrects the problem reported above in https://trac.sagemath.org/ticket/21231#comment:12

diff --git a/src/sage/interfaces/fricas.py b/src/sage/interfaces/fricas.py
index 8831c6c..5c1bc64 100644
--- a/src/sage/interfaces/fricas.py
+++ b/src/sage/interfaces/fricas.py
@@ -201,6 +201,7 @@ from __future__ import print_function

from sage.interfaces.tab_completion import ExtraTabCompletion
from sage.interfaces.expect import Expect, ExpectElement, FunctionElement, ExpectFunction
+from sage.misc.multireplace import multiple_replace
from sage.env import DOT_SAGE
import re
import six
@@ -571,6 +572,31 @@ class FriCASElement(ExpectElement):
raise IndexError("index out of range")
return self.elt(n+1)

+    def _latex_(self):
+        r"""
+        EXAMPLES::
+
+            sage: a = fricas(1/2) #optional - fricas
+            sage: latex(a)       #optional - fricas
+             1 \over 2
+
+        """
+        self._check_valid()
+        P = self.parent()
+        s = P._eval_line('outputAsTex(%s)'%self.name())
+        if not '$$' in s: + raise RuntimeError("Error texing axiom object.") + i = s.find('$$')
+        j = s.rfind('$$') + s = s[i+2:j] + s = multiple_replace({'\r':'', '\n':' ', + ' \\sp ':'^', + '\\arcsin ':'\\sin^{-1} ', + '\\arccos ':'\\cos^{-1} ', + '\\arctan ':'\\tan^{-1} '}, + re.sub(r'\\leqno$$.*?$$','',s)) # no eq number! + return s + def __int__(self): return int(self.sage())  I am not sure of the relevance of the particular substitutions that were performed by the old axiom.py code. Sorry for just including a patch but I am not sure whether or not I could have just pushed this change to trac since this branch seems to be private. ### comment:21 Changed 5 years ago by bpage FriCAS uses sage: fricas('asin(x)') asin(x) sage: fricas('acos(x)') acos(x) sage: fricas('atan(x)') atan(x)  Sage replaces asin, acos and atan with arcsin, arccos and arctan sage: asin(x) arcsin(x) sage: acos(x) arccos(x) sage: atan(x) arctan(x)  But FriCAS does not use these names so the following results in an error: sage: fricas(asin(x)) ... There are no library operations named arcsin Use HyperDoc Browse or issue )what op arctan to learn if there is any operation containing " arctan " in its name. ... sage: fricas(acos(x)) ... There are no library operations named arccos Use HyperDoc Browse or issue )what op arctan to learn if there is any operation containing " arctan " in its name. ... sage: fricas(atan(x)) ... There are no library operations named arctan Use HyperDoc Browse or issue )what op arctan to learn if there is any operation containing " arctan " in its name. ...  ### comment:22 in reply to: ↑ 20 ; follow-up: ↓ 23 Changed 5 years ago by mantepse Sorry for just including a patch but I am not sure whether or not I could have just pushed this change to trac since this branch seems to be private. Putting patches here is perfect for me, actually! Many thanks! ### comment:23 in reply to: ↑ 22 Changed 5 years ago by dimpase Replying to mantepse: Sorry for just including a patch but I am not sure whether or not I could have just pushed this change to trac since this branch seems to be private. Putting patches here is perfect for me, actually! Many thanks! although you could have just pushed your own (i.e. u/bpage) or public (i.e. public/) branch; then your commit(s) could be cherry-picked. An advantage is that it's no error-prone cutting/pasting, (and it is known whom to blame :-)). ### comment:24 Changed 5 years ago by git • Commit changed from 148bf77e2d3504f9d3dbbda038ca4270dd81bd83 to ca3277c6413ee5fb08e43f2ee48cf26fec5ec294 Branch pushed to git repo; I updated commit sha1. New commits:  ​ca3277c fix eval ### comment:25 Changed 5 years ago by mantepse • Status changed from new to needs_review ### comment:26 Changed 5 years ago by bpage Typeset output from FriCAS in the notebook is not working. Apparently the most recent commit is still missing _latex_. ### comment:27 Changed 5 years ago by bpage I think the markers |startKeyedMsg| and |endOfKeyedMsg| look strange in the error message and traceback. In the notebook only |endOfKeyedMsg| is shown as the output until clicking to expand the traceback. These markers should probably be removed from the output before displaying the message to the user. ### comment:28 Changed 5 years ago by mantepse Thanks for reminding me of LaTeX, I'm embarassed - I forgot! Removing |startKeyedMsg| and |endOfKeyedMsg| and the like is not completely trivial, but I'll try! ### comment:29 Changed 5 years ago by mantepse • Status changed from needs_review to needs_work ### comment:30 Changed 5 years ago by git • Commit changed from ca3277c6413ee5fb08e43f2ee48cf26fec5ec294 to b5f5a1c8ca4699a8d55cfa37ed9813b89f84ac5d Branch pushed to git repo; I updated commit sha1. New commits:  ​b5f5a1c fix latex, translate Factored, conversions for expressions ### comment:31 follow-up: ↓ 37 Changed 5 years ago by bpage Thanks, Martin. Here is a small patch that fixes a few problems with FriCAS-generated LaTeX. For example: sage: latex(fricas("integrate(sin(x+1/x),x)")) \int ^{\displaystyle x} {{\sin \left( {{{{{ \%A} ^{2}}+1} \over \%A}} \right)} \ {d \%A}}  and also subscripts and subscripts in a few other cases. diff --git a/src/sage/interfaces/fricas.py b/src/sage/interfaces/fricas.py index 8d2ac3f..594d5b8 100644 --- a/src/sage/interfaces/fricas.py +++ b/src/sage/interfaces/fricas.py @@ -598,7 +598,10 @@ class FriCASElement(ExpectElement): j = s.rfind('$$')
s = s[i+2:j]
s = multiple_replace({'\r':'', '\n':' ',
-                              ' \\sp ':'^',
+                              '\\sp ':'^',
+                              '\\sp{':'^{',
+                              '\\sb ':'_',
+                              '\\sb{':'_{',
'\\arcsin ':'\\sin^{-1} ',
'\\arccos ':'\\cos^{-1} ',
'\\arctan ':'\\tan^{-1} '},


### comment:32 Changed 5 years ago by git

• Commit changed from b5f5a1c8ca4699a8d55cfa37ed9813b89f84ac5d to ccb26de5806aa228e66ca44b47e5611dcf41b58e

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

 ​ccb26de Merge branch 'develop' of git://trac.sagemath.org/sage into fricas-interface

### comment:33 Changed 5 years ago by mantepse

• Status changed from needs_work to needs_review

### comment:34 follow-up: ↓ 41 Changed 5 years ago by mantepse

Hi Bill,

I don't understand your patch: why would you replace \sp{ by ^{ if you replace \sp by ^ anyway. Also, isn't \sp valid LaTeX?

But more importantly: shouldn't this patch really applied to FriCAS itself?

### comment:35 Changed 5 years ago by hemmecke

Maybe tex.spad from my mathjax branch might be interesting. It spits out true latex. https://github.com/hemmecke/fricas/commits/mathjax But maybe you really want mathjax or even 1d output of FriCAS objects, then look into mathjax.spad or 1d.spad. Most importantly, the output form can be changed at runtime.

Note that these files are GPL3+ and are therefore not (yet) part of FriCAS.

### comment:36 follow-up: ↓ 42 Changed 5 years ago by mantepse

Hi Bill and Ralf!

I'd like to get this ticket in soon (and in particular, stop working on it very soon). So if FriCAS produces only TeX currently, I'd like to leave it at this. As soon as the new tex.spad is in FriCAS, we can pick it up here, OK?

### comment:37 in reply to: ↑ 31 ; follow-up: ↓ 43 Changed 5 years ago by mantepse

                               '\\arcsin ':'\\sin^{-1} ',
'\\arccos ':'\\cos^{-1} ',
'\\arctan ':'\\tan^{-1} '},


another question: why are you replacing \arctan and friends? As far as I know, this is perfectyl valid LaTeX, no?

### comment:38 Changed 5 years ago by hemmecke

Yes, it is. And given the usual meaning of sin2(x) = (sin(x))2 and not sin(sin(x)), I even think that writing sin(-1) for arcsin is even confusing, no?

### comment:39 Changed 5 years ago by git

• Commit changed from ccb26de5806aa228e66ca44b47e5611dcf41b58e to 8871cb226009084007a60c30dbe62e4a28e83fc8

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

 ​8871cb2 tiny latex fix

### comment:40 Changed 5 years ago by git

• Commit changed from 8871cb226009084007a60c30dbe62e4a28e83fc8 to 2fe8ea595ef1f63ccc4d67cefe5688d314b95cdf

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

 ​2fe8ea5 forgot to raise NotImplementedError

### comment:41 in reply to: ↑ 34 Changed 5 years ago by bpage

Hi Bill,

I don't understand your patch: why would you replace \sp{ by ^{ if you replace \sp by ^ anyway.

Note the space '\sp '. It is looking for termination of \sp.

Also, isn't \sp valid LaTeX?

Not as far as I know - maybe TeX? In any case it does not work in Sage notebook.

But more importantly: shouldn't this patch really applied to FriCAS itself?

Perhaps. Waldek would probably accept a patch to that effect.

### comment:42 in reply to: ↑ 36 Changed 5 years ago by bpage

Hi Bill and Ralf!

I'd like to get this ticket in soon (and in particular, stop working on it very soon). So if FriCAS produces only TeX currently, I'd like to leave it at this. As soon as the new tex.spad is in FriCAS, we can pick it up here, OK?

I think Waldek is unlikely to change his position on the exclusion of copyleft licensed code in FriCAS since FriCAS already has a more permissive license. Of course that does not prevent its inclusion in the FriCAS external package here if someone were so inclined.

### comment:43 in reply to: ↑ 37 Changed 5 years ago by bpage

another question: why are you replacing \arctan and friends? As far as I know, this is perfectyl valid LaTeX, no?

Yes. I agree with this change. I just copied (with some reservations) what was added to the Axiom interface.

### comment:44 Changed 5 years ago by bpage

Error output in Sage notebook still looks like this

fricas("something stupid")

Traceback (click to the left of this block for traceback)
...
|endOfKeyedMsg|


Expanded:

Traceback (most recent call last):
File "<stdin>", line 1, in <module>
...
File "/home/wspage/sage/local/lib/python2.7/site-packages/sage/interfaces/fricas.py", line 399, in set
raise RuntimeError("An error occurred when FriCAS evaluated '%s': %s" % (value, output))
TypeError: An error occurred when FriCAS evaluated 'something stupid': |startKeyedMsg|
There are no library operations named something
Use HyperDoc Browse or issue
)what op something
to learn if there is any operation containing " something " in its
name.
|endOfKeyedMsg|

|startKeyedMsg|
Cannot find a definition or applicable library operation named
something with argument type(s)
Variable(stupid)

Perhaps you should use "@" to indicate the required return type, or

### comment:53 Changed 5 years ago by bpage

Well this is a slightly different but related topic: I prefer to treat Sage notebook cells beginning with %fricas like .input files rather than command lines. Because .input language syntax is somewhat more general it allows FriCAS code such as

for i in 1..10 repeat
if i<5 then
output i
else
output i+1


I notice that you already have code that allows creation and execution of .input files for very long expressions. Making _eval_line always create .input files would not be difficult if you override it in fricas.py.

### comment:54 Changed 5 years ago by git

• Commit changed from 41aefd7bcd18edbbee79a88ba8321e8f689f2fa0 to 63052797a0e42d36ee7c93cdf484506651c789f5

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

 ​6305279 try to make reviewer happier...

### comment:55 Changed 5 years ago by bpage

• Status changed from needs_work to positive_review

Reviewer is now very happy. :) Maybe we can consider .input format cells in a later ticket.

### comment:56 follow-up: ↓ 57 Changed 5 years ago by mantepse

There is actually an easy way to achieve what you want: First say:

fricas._eval_using_file_cutoff=30


(I don't know what a good value is. Don't make it 0 or 1.) and then:

%fricas
for i in 100000..100010 repeat
if i<5 then
output i
else
output partition(i+1)


Note however, that the result will not be displayed one by one, but only after the full thing is finished. I hate that.

### comment:57 in reply to: ↑ 56 ; follow-up: ↓ 58 Changed 5 years ago by bpage

There is actually an easy way to achieve what you want: First say:

fricas._eval_using_file_cutoff=30


(I don't know what a good value is. Don't make it 0 or 1.)

Nice! In fact

fricas._eval_using_file_cutoff=1


seems to work for me. Did you have a problem setting it to 1?

I could be happy with just such a flag option, especially if it was a bit more obvious like:

fricas.inputFormat(true)


But why not make it the default? Would you expect a serious performance issue?

Note however, that the result will not be displayed one by one, but only after the full thing is finished. I hate that.

Of course, but this is not a problem in one of the most important uses: defining a long function.

### comment:58 in reply to: ↑ 57 Changed 5 years ago by mantepse

Nice! In fact

fricas._eval_using_file_cutoff=1


seems to work for me. Did you have a problem setting it to 1?

Yes, but that might have been noise.

I could be happy with just such a flag option, especially if it was a bit more obvious like:

fricas.inputFormat(true)


Not sure yet, especially about the user interface.

But why not make it the default? Would you expect a serious performance issue?

Yes.

Note however, that the result will not be displayed one by one, but only after the full thing is finished. I hate that.

Of course, but this is not a problem in one of the most important uses: defining a long function.

In case you have time there are two major things that remain to be done (on separate tickets):

• find out how Waldek's fricas-as-library works.
• find out how to make fricas._eval_line and fricas.eval print stuff they receive immediately, until a special marker is encountered. (We can control the special marker via |$ioHook|, but for the moment let's pretend it is |startAlgebraOutput|. To be honest, I don't even know whether this is possible. I don't want to modify this ticket much further, unless the patchbot requires me to do so, to have it merged soon. ### comment:59 Changed 5 years ago by git • Commit changed from 63052797a0e42d36ee7c93cdf484506651c789f5 to 2231cea79d5c2c11e3520b623c7b5f2a19548b7a • Status changed from positive_review to needs_review Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:  ​1347786 Add jump_number() to posets. ​3e6ef52  to spaces. ​fe815c8 Merge branch 'u/jmantysalo/jump_number' of git://trac.sagemath.org/sage into fricas-interface ​aa04d96 Add algorithm-keyword to canonical labeling of posets. ​d923dc1 Merge branch 'u/jmantysalo/posets__add_algorithm_keyword_to_canonical_relabel__' of git://trac.sagemath.org/sage into fricas-interface ​2231cea beautify documentation ### comment:60 Changed 5 years ago by mantepse • Status changed from needs_review to positive_review I only changed *really* minor documentation stuff. ### comment:61 Changed 5 years ago by chapoton reviewer name must be *full real name* ### comment:62 Changed 5 years ago by vbraun • Status changed from positive_review to needs_work ### comment:63 Changed 5 years ago by mantepse Is it "needs_work" because of the missing reviewer name? (the patchbot doesn't like it because it cannot fetch the branch, that's not my fault I believe) ### comment:64 Changed 5 years ago by bpage • Reviewers changed from bpage to Bill Page • Status changed from needs_work to positive_review I am unsure of the correct protocol when setting/re-setting this "positive review" flag ... ### comment:65 Changed 5 years ago by vbraun • Status changed from positive_review to needs_work sage -t --long src/sage/interfaces/fricas.py ********************************************************************** File "src/sage/interfaces/fricas.py", line 971, in sage.interfaces.fricas.FriCASElement._sage_ Failed example: fricas("integrate(sin((x^2+1)/x),x)").sage() Exception raised: Traceback (most recent call last): File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 498, in _run self.compile_and_execute(example, compiler, test.globs) File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 861, in compile_and_execute exec(compiled, globs) File "<doctest sage.interfaces.fricas.FriCASElement._sage_[1]>", line 1, in <module> fricas("integrate(sin((x^2+1)/x),x)").sage() File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/interfaces/interface.py", line 244, in __call__ return cls(self, x, name=name) File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/interfaces/expect.py", line 1382, in __init__ raise_(TypeError, x, sys.exc_info()[2]) File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/interfaces/expect.py", line 1377, in __init__ self._name = parent._create(value, name=name) File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/interfaces/interface.py", line 434, in _create self.set(name, value) File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/interfaces/fricas.py", line 506, in set output = self.eval(cmd, reformat=False) File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/interfaces/fricas.py", line 596, in eval **kwds) File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/interfaces/expect.py", line 1294, in eval for L in code.split('\n') if L != '']) File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/interfaces/expect.py", line 905, in _eval_line self._start() File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/interfaces/fricas.py", line 273, in _start Expect._start(self) File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/interfaces/expect.py", line 486, in _start (self.name(), cmd, e, self._install_hints())) TypeError: unable to start fricas because the command 'fricas -nox -noclef' failed: The command was not found or was not executable: fricas. ********************************************************************** 1 item had failures: 1 of 3 in sage.interfaces.fricas.FriCASElement._sage_ [3 tests, 1 failure, 0.19 s]  ### comment:66 Changed 5 years ago by git • Commit changed from 2231cea79d5c2c11e3520b623c7b5f2a19548b7a to c651222036f3a60e3d55a52423dc0e325d469e95 Branch pushed to git repo; I updated commit sha1. New commits:  ​92378ca Fix for sage shell with TERM=dumb ​fc0b081 Merge branch 'u/mkoeppe/sage_mode_for_emacs_has_display_problem_in_sage_7_4_beta0' of git://trac.sagemath.org/sage into fricas-interface ​c651222 fix missing optional in doctests ### comment:67 Changed 5 years ago by mantepse Thanks, fixed. I'm very sorry, but my git-foo is very weak, so there are some commits in the branch which do not belong. If there is an easy way to get rid of them, please let me know... They are: ​43c1136 Exclude plain text from IPython super call ​b027498 Merge branch 'u/vbraun/call_repr_once' of git://trac.sagemath.org/sage into develop ​1347786 Add jump_number() to posets. ​3e6ef52 <tab> to spaces. ​fe815c8 Merge branch 'u/jmantysalo/jump_number' of git://trac.sagemath.org/sage into fricas-interface ​aa04d96 Add algorithm-keyword to canonical labeling of posets. ​d923dc1 Merge branch 'u/jmantysalo/posets__add_algorithm_keyword_to_canonical_relabel__' of git://trac.sagemath.org/sage into fricas-interface ​92378ca Fix for sage shell with TERM=dumb ​fc0b081 Merge branch 'u/mkoeppe/sage_mode_for_emacs_has_display_problem_in_sage_7_4_beta0' of git://trac.sagemath.org/sage into fricas-interface  Or should I just create a new branch and copy the good stuff into that new branch? ### comment:68 Changed 5 years ago by vbraun To undo the merge you have to go back to before it: git reset --hard 2231cea79d5c2c11e3520b623c7b5f2a19548b7a  and then only add the commit you want git cherry-pick c651222036f3a60e3d55a52423dc0e325d469e95  and then (force) push the new branch ### comment:69 Changed 5 years ago by dimpase https://trac.sagemath.org/ticket/21209#comment:64 shows that you need to implement fricas.quit() here. ### comment:70 Changed 5 years ago by mantepse https://trac.sagemath.org/ticket/21209#comment:66 indicates that it is sufficient to append a \r to the quitstring. ### comment:71 Changed 5 years ago by mantepse • Branch changed from u/mantepse/21231 to u/mantepse/fricas_interface • Commit changed from c651222036f3a60e3d55a52423dc0e325d469e95 to c39474e6b94094d00979ff68aaf7671dc8747d92 ### comment:72 Changed 5 years ago by git • Commit changed from c39474e6b94094d00979ff68aaf7671dc8747d92 to c1baa025052f36a3426ff1e54a3042abb4fbd7ba Branch pushed to git repo; I updated commit sha1. New commits:  ​c1baa02 remove merge conflict leftover ### comment:73 Changed 5 years ago by git • Commit changed from c1baa025052f36a3426ff1e54a3042abb4fbd7ba to f6b0b4784999ef24228922dcc823d8d16edd08e9 Branch pushed to git repo; I updated commit sha1. New commits:  ​f6b0b47 indentation fixes ### comment:74 Changed 5 years ago by git • Commit changed from f6b0b4784999ef24228922dcc823d8d16edd08e9 to 835e519de78fb4650a6fdcdaa21d7a17b2c6ff2f Branch pushed to git repo; I updated commit sha1. New commits:  ​835e519 try to make the patchbot happy ### comment:75 Changed 5 years ago by mantepse • Status changed from needs_work to needs_review ### comment:76 follow-up: ↓ 77 Changed 5 years ago by bpage There is something strange about the way FriCAS matrices are being typeset. sage: latex(fricas("matrix([[1,2],[3,4]])")) {\left[ 1, \: 2 \right]} \ {\left[ 3, \: 4 \right]}  but sage: fricas("matrix([[1,2],[3,4]])::TEX") ["$$", "\left[", "\begin{array}{cc}", "1 & 2 \\ ", "3 & 4 ", "\end{array}", "\right]", "$$"]  However in other cases it looks normal. For example: sage: latex(fricas("integrate(sin(x/(1+sqrt(x))),x)")) \int ^{\displaystyle x} {{\sin \left( {{ \%A \over {{\sqrt { \%A}}+1}}} \right)} \ {d \%A}} sage: fricas("integrate(sin(x/(1+sqrt(x))),x)::TEX") ["$$", "\int \sp{\displaystyle x} {{\sin ", "\left(", "{{ \%A \over {{\sqrt { \%A}}+1}}} ", "\right)}", "\ {d \%A}} ", "$$"]  ### comment:77 in reply to: ↑ 76 Changed 5 years ago by mantepse Replying to bpage: There is something strange about the way FriCAS matrices are being typeset. sage: latex(fricas("matrix([[1,2],[3,4]])")) {\left[ 1, \: 2 \right]} \ {\left[ 3, \: 4 \right]}  but sage: fricas("matrix([[1,2],[3,4]])::TEX") ["$$", "\left[", "\begin{array}{cc}", "1 & 2 \\ ", "3 & 4 ", "\end{array}", "\right]", "$$"]  I am using outputAsTex. Should I use ::TEX instead? ### comment:78 Changed 5 years ago by bpage I am perplexed that outputAsTex does not produce the same thing as ::TEX. I was not aware of that. Perhaps this should be reported as a bug in FriCAS? In any case it seems that the FriCAS command: )set output tex on  uses the latter method. (1) -> outputAsTex matrix [[1,2],[3,4]] $${\left[ 1, \: 2 \right]} \ {\left[ 3, \: 4 \right]} \leqno(1)$$ Type: Void (2) -> matrix([[1,2],[3,4]])::TEX (2) ["$$", "\left[", "\begin{array}{cc}", "1 & 2 \\ ", "3 & 4 ", "\end{array}", "\right]", "$$"] Type: TexFormat (3) -> )set output tex on (3) -> matrix([[1,2],[3,4]]) +1 2+ (3) | | +3 4+ $$\left[ \begin{array}{cc} 1 & 2 \\ 3 & 4 \end{array} \right] \leqno(3)$$ Type: Matrix(Integer)  I think it is reasonable to expect that typeset mode with the FriCAS interface would produce the same result. ### comment:79 Changed 5 years ago by mantepse ping? ### comment:80 Changed 5 years ago by git • Commit changed from 835e519de78fb4650a6fdcdaa21d7a17b2c6ff2f to 9a049fc37c5d0bc381ecc58364bc9706485646c5 Branch pushed to git repo; I updated commit sha1. New commits:  ​102263d Merge branch 'develop' of git://trac.sagemath.org/sage into u/mantepse/fricas_interface ​9a049fc improve latex, various doctest fixes ### comment:81 Changed 5 years ago by bpage Thanks for the update. The output of sage: latex(fricas("matrix(1,2],[3,4?)")) \left[ \begin{array}{cc} 1 & 2 3 & 4 \end{array} \right] now looks good but ... In sage --notebook the output from a cell beginning with %fricas is no longer being typeset even though the [x]Typeset option is checked. ### comment:82 Changed 5 years ago by git • Commit changed from 9a049fc37c5d0bc381ecc58364bc9706485646c5 to 601f8fe5eac80f5a4bc4a274e5505091beb7a0c4 Branch pushed to git repo; I updated commit sha1. New commits:  ​923071b improve doc ​cbcd523 Merge branch 'u/mantepse/fricas_interface' of git://trac.sagemath.org/sage into u/mantepse/fricas_interface ​601f8fe fix bug in latex (and elsewhere), refactor ### comment:83 Changed 5 years ago by bpage • Status changed from needs_review to positive_review My comment about how typeset mode works in the a Sage worksheet was incorrect. Everything looks good! Thanks. ### comment:84 follow-up: ↓ 85 Changed 5 years ago by vbraun • Status changed from positive_review to needs_work Merge conflict ### comment:85 in reply to: ↑ 84 Changed 5 years ago by bpage Replying to vbraun: Merge conflict I just did wspage@strix ~/sage$ git branch
develop
master
* t/21231/fricas_interface
wspage@strix ~/sage $git pull remote: Counting objects: 1865, done. remote: Compressing objects: 100% (794/794), done. remote: Total 1291 (delta 1146), reused 618 (delta 493) Receiving objects: 100% (1291/1291), 190.57 KiB | 175.00 KiB/s, done. Resolving deltas: 100% (1146/1146), completed with 398 local objects. From git://trac.sagemath.org/sage 0ae5fd8..c5dadf9 develop -> trac/develop 62b052e..b57c07f u/chapoton/21210 -> trac/u/chapoton/21210 eda9412..c76bdfa u/chapoton/21523 -> trac/u/chapoton/21523 41d35fc..9114ec8 u/chapoton/21577 -> trac/u/chapoton/21577 * [new branch] u/jdemeyer/gp2c_does_not_pass_self_checks -> trac/u/jdemeyer/gp2c_does_not_pass_self_checks + f54ecb5...ccd9c1e u/jdemeyer/pari__use_prot_none_for_unused_virtual_stack_memory -> trac/u/jdemeyer/pari__use_prot_none_for_unused_virtual_stack_memory (forced update) * [new tag] 7.4.beta6 -> 7.4.beta6 Already up-to-date.  and did not see any conflict. How can I see what is wrong? ### comment:86 Changed 5 years ago by git • Commit changed from 601f8fe5eac80f5a4bc4a274e5505091beb7a0c4 to 08463b9e711e44bf59017a2578e120e37e7eb600 Branch pushed to git repo; I updated commit sha1. New commits:  ​08463b9 Merge branch 'develop' of git://trac.sagemath.org/sage into u/mantepse/fricas_interface ### comment:87 Changed 5 years ago by mantepse • Status changed from needs_work to positive_review fixed trivial merge conflict. ### comment:88 Changed 5 years ago by git • Commit changed from 08463b9e711e44bf59017a2578e120e37e7eb600 to 54cc3ef8e290f7f6af7a24777989f402067ec564 • Status changed from positive_review to needs_review Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:  ​54cc3ef fix a few "::" in documentation. ### comment:89 Changed 5 years ago by charpent • Reviewers changed from Bill Page to Bill Page Emmanuel Charpentier • Status changed from needs_review to positive_review Passes ptestlong on top of 7.4beta6 (+#21622, mandatory for getting Sage to compile) with one failure : ---------------------------------------------------------------------- sage -t --long --warn-long 113.7 src/sage/homology/simplicial_complex.py # 1 doctest failed ----------------------------------------------------------------------  which passes standalone : charpent@asus16-ec:/usr/local/sage-7$ sage -t --long --warn-long 113.7 src/sage/homology/simplicial_complex.py
Running doctests with ID 2016-10-02-13-43-39-bc494aee.
Git branch: t/21231/fricas_interface
Using --optional=atlas,database_gap,mpir,python2,sage,sage_mode
Doctesting 1 file.
sage -t --long --warn-long 113.7 src/sage/homology/simplicial_complex.py
[588 tests, 3.20 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 3.3 seconds
cpu time: 3.0 seconds
cumulative wall time: 3.2 seconds


This failure is a recurring known problem. ==> positive_review notwithstanding.

### comment:90 follow-up: ↓ 91 Changed 5 years ago by mantepse

There are a few docbuild errors, I'll correct them tomorrow morning.

### comment:91 in reply to: ↑ 90 ; follow-up: ↓ 92 Changed 5 years ago by charpent

There are a few docbuild errors,

Wups. I missed that (how did you find this ?).

I'll correct them tomorrow morning.

Ring me afterwards, I'll re-test it un the evening (European time).

### comment:92 in reply to: ↑ 91 Changed 5 years ago by mantepse

There are a few docbuild errors,

Wups. I missed that (how did you find this ?).

I only saw it on the patchbot report... (there is one one 7.4 beta6 by cristal which seems to report something real)

### comment:93 Changed 5 years ago by git

• Commit changed from 54cc3ef8e290f7f6af7a24777989f402067ec564 to 70c3acede9b908417aea31ab46bd4871ff62a98e
• Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

 ​70c3ace fix docbuild problems and improve doc

### comment:94 Changed 5 years ago by mantepse

• Reviewers changed from Bill Page Emmanuel Charpentier to Bill Page, Emmanuel Charpentier

Everything should be fine now!

### comment:95 Changed 5 years ago by charpent

• Status changed from needs_review to positive_review

Passes ptestlong with the same failure as before, which again passes standalone.

positive_review

### comment:96 Changed 5 years ago by git

• Commit changed from 70c3acede9b908417aea31ab46bd4871ff62a98e to ab4214adf600dbba32abc88c5fcd5d36945a4b14
• Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

 ​ab4214a Merge branch 'develop' of git://trac.sagemath.org/sage into u/mantepse/fricas_interface

### comment:97 Changed 5 years ago by dimpase

the latest push created a mega-patch for no good reason. What was the reason for this?

### comment:98 follow-up: ↓ 99 Changed 5 years ago by mantepse

I am sorry, I thought I am supposed to merge when a new develop appears. Could you tell me how to revert?

### comment:99 in reply to: ↑ 98 ; follow-up: ↓ 101 Changed 5 years ago by dimpase

I am sorry, I thought I am supposed to merge when a new develop appears. Could you tell me how to revert?

only if there are merge conflicts (which you would see by the Branch field in the ticket turning red).

Just revert the last commit (and push again, with -f switch). Not sure how to do this using git trac if you must use it... Let me know if you get stuck on this.

### comment:100 Changed 5 years ago by git

• Commit changed from ab4214adf600dbba32abc88c5fcd5d36945a4b14 to 70c3acede9b908417aea31ab46bd4871ff62a98e

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

Thank you!

### comment:102 Changed 5 years ago by dimpase

• Status changed from needs_review to positive_review

OK, good, back to the positively reviewed patch.

### comment:103 follow-ups: ↓ 105 ↓ 106 Changed 5 years ago by jdemeyer

• Status changed from positive_review to needs_work

You should not use a bare except: (see the end of src/sage/symbolic/integration/external.py). Moreover, I would suggest to replace

     try:
return result.sage()
except:
raise ValueError("Unable to parse: {}".format(result))


by

return result.sage()


### comment:104 Changed 5 years ago by git

• Commit changed from 70c3acede9b908417aea31ab46bd4871ff62a98e to e281742685eb891ff7883bd6e91b16e8c31b92d6

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

 ​e281742 suggestion by reviewer

### comment:105 in reply to: ↑ 103 Changed 5 years ago by mantepse

I agree - I only adapted this part of the code.

### comment:106 in reply to: ↑ 103 Changed 5 years ago by mantepse

could you set this back to positive review? I would really like to get this into sage soon, because there will be trivial, but tedious merge conflicts all the time otherwise. I'd be grateful...

### comment:107 Changed 5 years ago by jdemeyer

• Status changed from needs_work to needs_review

Well, if you don't set it to needs_review, I don't know that it's ready for review.

### comment:108 Changed 5 years ago by jdemeyer

• Status changed from needs_review to positive_review

### comment:109 Changed 5 years ago by vbraun

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