Opened 19 months ago
Closed 12 months ago
#31796 closed defect (fixed)
Make maxima(<string>) output parseable
Reported by:  Emmanuel Charpentier  Owned by:  

Priority:  major  Milestone:  sage9.5 
Component:  interfaces  Keywords:  maxima symbolics 
Cc:  Frédéric Chapoton, Nils Bruin, Samuel Lelièvre, Marius Gerbershagen  Merged in:  
Authors:  Emmanuel Charpentier, Matthias Koeppe, Frédéric Chapoton  Reviewers:  Samuel Lelièvre 
Report Upstream:  N/A  Work issues:  
Branch:  05fa319 (Commits, GitHub, GitLab)  Commit:  05fa319b65be9b31487bb493450510e32c2f0d07 
Dependencies:  Stopgaps: 
Description (last modified by )
(See also this sagedevel
post.)
Currently, all whitespace is stripped off maxima(x)
and maxima_calculus(x)
where x
is a string. The same goes for Maxima code defined via the interfaces. To illustrate :
r1=maxima_calculus('foo: a and (b or c)') r2=maxima_calculus('untree(x) := append([part(x, 0)], maplist(lambda([u], if atom(u) then u else untree(u)), x))') r3=maxima_calculus('untree(foo)') (r1, r2, r3)
outputs :
(aand(borc), untree(x):=append([part(x,0)],maplist(lambda([u],ifatom(u)thenuelseuntree(u)),x)), [\"and\",a,[\"or\",b,c]])
This also deminstrates that the Maxima interpretation of the first instruction is correct ; it is its output which is illinterpreted by Sage...
In order to maintain the print(eval(read(x))==x
equality, it is desirable to keep spaces at least around boolean operators.
Change History (33)
comment:2 Changed 19 months ago by
Branch:  → u/charpent/legible_maxima_output 

comment:3 Changed 19 months ago by
Cc:  Nils Bruin added 

Commit:  → 5f31eeba0240bd032737d1de4bb6b347b526afa9 
Status:  new → needs_review 
Replying to nbruin:
It actually looks like whitespace has been stripped right from the start from output in the maxima interface: https://github.com/sagemath/sage/blob/e76b97fedaaec412fb4cce98bc55eb3d0ba2697f/src/sage/interfaces/maxima.py#L562 (the blame of that line goes back to original commits)
If I recall correctly, there were problems with maxima inserting spaces at unpredictable places every now and again;
This still happens, at least for MaximaElement pieces. To be explored in a followup ticket (but I'm afraid that this one is in Maxima's territory...). I've installed a workaround for this in assumptions.py
even in the middle of identifiers!
This I didn't see (yet).
The original commits would have been for maxima on CLISP rather than on ECL (although I recall the whitespace problem occurring on ECL (too?)). It could be that with the different iterations of "expect"  and almost certainly in maxima_lib  this isn't a problem anymore, but it means you'll have to test a *lot* in order to make really sure that a very old problem doesn't inadvertently comes back.
Advice gladly accepted for filing and solving a followup ticket, or to better test the proposed solution.
For the latter : what would you think of generating random SR expressions, translate them to Maxima strings, and check backandforth translations ? ISTR that such a random expression generator exists somewhere in Sage but, for the life of me, I'm unable to recall where I saw this...
Also note that maxima_lib and maxima each have their own
_eval_line_
which does the stripping,
In different ways ==> two different fixes. No possible code sharing here without major restructuration.
so to keep the two interfaces in step, you'd want to change it in both.
Done. You're welcome to review my branch, which passes ptestlong
with exactly the same errors as develop
at 9.3.rc5
(and therefore needs_review
). And delivers parseable output for Maxima strings containing logical operators. Yay !
New commits:
5f31eeb  Ticket #31796 : make maxima output parseable.

comment:4 Changed 19 months ago by
Note : Lint
complains concern exclusively trailing whitespace
(a couple occurrences in my patch) and multiple statements on one line
(none in my patch).
Should I try and fix all these occurrences (at least as a public service) ? Or should this be a separate ticket ?
comment:5 followup: 8 Changed 19 months ago by
Reviewers:  → Samuel Lelièvre 

Status:  needs_review → needs_work 
There seems to be a copypaste mistake in the German
and Japanese tour_algebra.rst
, with the de2
block
replaced by the adapted de1
block rather than by the
adapted de2
block.
comment:7 Changed 19 months ago by
Commit:  5f31eeba0240bd032737d1de4bb6b347b526afa9 → d2059f7ca2d876213e938cc50fd19c5adbf15b8d 

Branch pushed to git repo; I updated commit sha1. New commits:
d2059f7  Ticket #31796: fix paste mistakes in german and japanese tutorials.

comment:8 Changed 19 months ago by
Replying to slelievre:
There seems to be a copypaste mistake in the German and Japanese
tour_algebra.rst
, with thede2
block replaced by the adaptedde1
block rather than by the adaptedde2
block.
Right. Damn... Fixed.
comment:9 Changed 19 months ago by
Authors:  → Emmanuel Charpentier 

Status:  needs_work → needs_review 
comment:10 Changed 19 months ago by
Description:  modified (diff) 

comment:11 followup: 12 Changed 19 months ago by
The patchbot's pyflakes plugin complains:
 src/sage/interfaces/maxima.py:498:1 'sage.env.DOT_SAGE' imported but unused  src/sage/interfaces/maxima.py:498:1 'sage.env.SAGE_LOCAL' imported but unused  src/sage/interfaces/maxima_abstract.py:997:9 local variable 'a' is assigned to but never used  src/sage/symbolic/expression_conversions.py:21:1 'sage.symbolic.constants.I' imported but unused  src/sage/symbolic/expression_conversions.py:1112:13 redefinition of unused 'I' from line 21
Not sure what to make of those, especially the first two.
Someone with an expert opinion on those please chime in.
comment:12 Changed 19 months ago by
Replying to slelievre:
The patchbot's pyflakes plugin complains:
 src/sage/interfaces/maxima.py:498:1 'sage.env.DOT_SAGE' imported but unused  src/sage/interfaces/maxima.py:498:1 'sage.env.SAGE_LOCAL' imported but unused
Indeed : neither DOT_SAGE
nor SAGE_LOCAL
appear anywhere in this file but on line 487, where they are imported from sage.env
.
Residues of code past ?
 src/sage/interfaces/maxima_abstract.py:997:9 local variable 'a' is assigned to but never used}}}
This one is right, but seems harmless. Was in the original code.
 src/sage/symbolic/expression_conversions.py:21:1 'sage.symbolic.constants.I' imported but unused  src/sage/symbolic/expression_conversions.py:1112:13 redefinition of unused 'I' from line 21
This one is hard to check. After hunting all capital I
in the file :
 In live code, I find an import of I in
AlgebraicConverter.arithmetic
, line 1112 (as announced bypyflakes
).  All the other uses are in examples/tests.
Past code residues, again ?
Not sure what to make of those, especially the first two.
Someone with an expert opinion on those please chime in.
I'm interested, too.
But I can also make a new commit including the changes suggested by pyflakes
(as well as deleting the "trailing whitespace" relint
complains about so insistently, but not the semicolon uses pycodestyle
hates...).
Advice ?
comment:13 Changed 19 months ago by
Cc:  Frédéric Chapoton Samuel Lelièvre added 

Status:  needs_review → positive_review 
Let us get this in; pyflakes and relint can be worked on in a followup ticket.
comment:14 followup: 16 Changed 18 months ago by
The ticket tests fine, but once you remove the $DOT_SAGE/maxima_commandlist_cache.sobj
then tests fail with (this took quite a long time to track down):
sage t long randomseed=0 src/sage/symbolic/maxima_wrapper.py ********************************************************************** File "src/sage/symbolic/maxima_wrapper.py", line 81, in sage.symbolic.maxima_wrapper.MaximaWrapper.__getattr__ Failed example: s.completions('u.airy_',globals()) Expected: ['u.airy_ai', 'u.airy_bi', 'u.airy_dai', 'u.airy_dbi'] Got: ['u.airy_ai', 'u.airy_bi', 'u.airy_dbi'] ********************************************************************** 1 item had failures: 1 of 7 in sage.symbolic.maxima_wrapper.MaximaWrapper.__getattr__ [30 tests, 1 failure, 1.40 s]  sage t long randomseed=0 src/sage/symbolic/maxima_wrapper.py # 1 doctest failed 
How about we just version it as maxima_commandlist_cache_v2.sobj
comment:15 Changed 18 months ago by
Status:  positive_review → needs_work 

comment:16 Changed 18 months ago by
How does versioning the command list help for this? It looks to be that there's a parsing problem that leads to airy_dai
not being discovered as a valid function in maxima. Shouldn't the fix consist of fixing the parsing? I don't think we are shipping maxima_commandlist_cache, so sage should be able to build the cache reliable and correctly by itself.
comment:17 Changed 17 months ago by
Milestone:  sage9.4 → sage9.5 

Setting a new milestone for this ticket based on a cursory review.
comment:18 Changed 16 months ago by
Branch:  u/charpent/legible_maxima_output → u/mkoeppe/legible_maxima_output 

comment:19 Changed 16 months ago by
Commit:  d2059f7ca2d876213e938cc50fd19c5adbf15b8d → a3aa3c93405b014e9facc97b6363a85faacdc4c4 

comment:20 Changed 16 months ago by
Commit:  a3aa3c93405b014e9facc97b6363a85faacdc4c4 → 767e89a1480f828a71ec32d5007e4bfc77b2e649 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
767e89a  Ticket #31796 : make maxima output parseable.

comment:21 Changed 16 months ago by
rm ~/.sage/maxima_commandlist_cache.sobj && ./sage t long randomseed=0 src/sage/symbolic/maxima_wrapper.py
gives a different failure here:
File "src/sage/symbolic/maxima_wrapper.py", line 62, in sage.symbolic.maxima_wrapper.MaximaWrapper.__init__ Failed example: s.completions('u.elliptic_',globals()) Expected: ['u.elliptic_e', 'u.elliptic_ec', 'u.elliptic_eu', 'u.elliptic_f', 'u.elliptic_kc', 'u.elliptic_pi'] Got: ['u.elliptic_ec', 'u.elliptic_eu', 'u.elliptic_f', 'u.elliptic_kc', 'u.elliptic_pi']
comment:22 Changed 16 months ago by
Commit:  767e89a1480f828a71ec32d5007e4bfc77b2e649 → d7abbe62be2a4bbb9262963de68bb0d0cf9cbcfe 

Branch pushed to git repo; I updated commit sha1. New commits:
d7abbe6  MaximaAbstract._commands: Remove whitespace in apropos output before breaking it apart

comment:23 Changed 16 months ago by
Authors:  Emmanuel Charpentier → Emmanuel Charpentier, Matthias Koeppe 

Status:  needs_work → needs_review 
comment:24 Changed 15 months ago by
Cc:  Marius Gerbershagen added 

comment:26 Changed 13 months ago by
Status:  needs_review → needs_work 

comment:27 Changed 12 months ago by
Branch:  u/mkoeppe/legible_maxima_output → public/ticket/31796 

Commit:  d7abbe62be2a4bbb9262963de68bb0d0cf9cbcfe → 9e483652d0fe72ca59c5af56daacf4c56955b507 
Status:  needs_work → needs_review 
comment:28 Changed 12 months ago by
Authors:  Emmanuel Charpentier, Matthias Koeppe → Emmanuel Charpentier, Matthias Koeppe, Frédéric Chapoton 

comment:29 Changed 12 months ago by
Commit:  9e483652d0fe72ca59c5af56daacf4c56955b507 → 05fa319b65be9b31487bb493450510e32c2f0d07 

Branch pushed to git repo; I updated commit sha1. New commits:
05fa319  some more details

comment:30 followup: 31 Changed 12 months ago by
ok, looks good. Patchbot is morally green.
I vote for positive review. Any confirmation from other reviewers or authors ?
comment:31 followup: 32 Changed 12 months ago by
Replying to chapoton:
ok, looks good. Patchbot is morally green.
What means "morally" green ???
I vote for positive review. Any confirmation from other reviewers or authors ?
Running ptestlong
once more (just in case : this particular patch has already proven notsoeasy to integrate). Stay tuned...
comment:32 Changed 12 months ago by
Status:  needs_review → positive_review 

Replying to charpent:
Replying to chapoton:
ok, looks good. Patchbot is morally green.
What means "morally" green ???
I vote for positive review. Any confirmation from other reviewers or authors ?
Running
ptestlong
once more (just in case : this particular patch has already proven notsoeasy to integrate). Stay tuned...
On Debian testing running on core i7 + 16 GB RAM, running ptestlong
on this patch added on top of 9.5.beta7 gives no new failure.
==> positive_review
is OK for me. I set it, but feel free to unset if necessary.
comment:33 Changed 12 months ago by
Branch:  public/ticket/31796 → 05fa319b65be9b31487bb493450510e32c2f0d07 

Resolution:  → fixed 
Status:  positive_review → closed 
It actually looks like whitespace has been stripped right from the start from output in the maxima interface: https://github.com/sagemath/sage/blob/e76b97fedaaec412fb4cce98bc55eb3d0ba2697f/src/sage/interfaces/maxima.py#L562 (the blame of that line goes back to original commits)
If I recall correctly, there were problems with maxima inserting spaces at unpredictable places every now and again; even in the middle of identifiers! The original commits would have been for maxima on CLISP rather than on ECL (although I recall the whitespace problem occurring on ECL (too?)). It could be that with the different iterations of "expect"  and almost certainly in maxima_lib  this isn't a problem anymore, but it means you'll have to test a *lot* in order to make really sure that a very old problem doesn't inadvertently comes back.
Also note that maxima_lib and maxima each have their own
_eval_line_
which does the stripping, so to keep the two interfaces in step, you'd want to change it in both.