#31796 closed defect (fixed)

Make maxima(<string>) output parseable

Reported by: Emmanuel Charpentier Owned by:
Priority: major Milestone: sage-9.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:

Status badges

Description (last modified by Emmanuel Charpentier)

(See also this sage-devel 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 ill-interpreted 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:1 Changed 19 months ago by Nils Bruin

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.

Last edited 19 months ago by Nils Bruin (previous) (diff)

comment:2 Changed 19 months ago by Emmanuel Charpentier

Branch: u/charpent/legible_maxima_output

comment:3 in reply to:  1 Changed 19 months ago by Emmanuel Charpentier

Cc: Nils Bruin added
Commit: 5f31eeba0240bd032737d1de4bb6b347b526afa9
Status: newneeds_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 back-and-forth 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:

5f31eebTicket #31796 : make maxima output parseable.

comment:4 Changed 19 months ago by Emmanuel Charpentier

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 Changed 19 months ago by Samuel Lelièvre

Reviewers: Samuel Lelièvre
Status: needs_reviewneeds_work

There seems to be a copy-paste 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:6 Changed 19 months ago by Samuel Lelièvre

Please fill in author name so patchbots run.

comment:7 Changed 19 months ago by git

Commit: 5f31eeba0240bd032737d1de4bb6b347b526afa9d2059f7ca2d876213e938cc50fd19c5adbf15b8d

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

d2059f7Ticket #31796: fix paste mistakes in german and japanese tutorials.

comment:8 in reply to:  5 Changed 19 months ago by Emmanuel Charpentier

Replying to slelievre:

There seems to be a copy-paste 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.

Right. Damn... Fixed.

comment:9 in reply to:  6 Changed 19 months ago by Emmanuel Charpentier

Authors: Emmanuel Charpentier
Status: needs_workneeds_review

Replying to slelievre:

Please fill in author name so patchbots run.

Done. Thank you !

comment:10 Changed 19 months ago by Emmanuel Charpentier

Description: modified (diff)

comment:11 Changed 19 months ago by Samuel Lelièvre

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 in reply to:  11 Changed 19 months ago by Emmanuel Charpentier

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 by pyflakes).
  • 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 Samuel Lelièvre

Cc: Frédéric Chapoton Samuel Lelièvre added
Status: needs_reviewpositive_review

Let us get this in; pyflakes and relint can be worked on in a follow-up ticket.

comment:14 Changed 18 months ago by Volker Braun

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 --random-seed=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 --random-seed=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 Volker Braun

Status: positive_reviewneeds_work

comment:16 in reply to:  14 Changed 18 months ago by Nils Bruin

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 Matthias Köppe

Milestone: sage-9.4sage-9.5

Setting a new milestone for this ticket based on a cursory review.

comment:18 Changed 16 months ago by Matthias Köppe

Branch: u/charpent/legible_maxima_outputu/mkoeppe/legible_maxima_output

comment:19 Changed 16 months ago by Matthias Köppe

Commit: d2059f7ca2d876213e938cc50fd19c5adbf15b8da3aa3c93405b014e9facc97b6363a85faacdc4c4

rebased on 9.4.rc2


New commits:

a3aa3c9Ticket #31796 : make maxima output parseable.

comment:20 Changed 16 months ago by git

Commit: a3aa3c93405b014e9facc97b6363a85faacdc4c4767e89a1480f828a71ec32d5007e4bfc77b2e649

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

767e89aTicket #31796 : make maxima output parseable.

comment:21 Changed 16 months ago by Matthias Köppe

rm ~/.sage/maxima_commandlist_cache.sobj && ./sage -t --long --random-seed=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 git

Commit: 767e89a1480f828a71ec32d5007e4bfc77b2e649d7abbe62be2a4bbb9262963de68bb0d0cf9cbcfe

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

d7abbe6MaximaAbstract._commands: Remove whitespace in apropos output before breaking it apart

comment:23 Changed 16 months ago by Matthias Köppe

Authors: Emmanuel CharpentierEmmanuel Charpentier, Matthias Koeppe
Status: needs_workneeds_review

comment:24 Changed 15 months ago by Matthias Köppe

Cc: Marius Gerbershagen added

comment:25 Changed 13 months ago by Frédéric Chapoton

patchbot reports some trivial failing doctests in it tutorial

comment:26 Changed 13 months ago by Frédéric Chapoton

Status: needs_reviewneeds_work

comment:27 Changed 12 months ago by Frédéric Chapoton

Branch: u/mkoeppe/legible_maxima_outputpublic/ticket/31796
Commit: d7abbe62be2a4bbb9262963de68bb0d0cf9cbcfe9e483652d0fe72ca59c5af56daacf4c56955b507
Status: needs_workneeds_review

I have fixed the doctests in italian documentation


New commits:

d3ec3aaMerge branch 'u/mkoeppe/legible_maxima_output' in 9.5.b7
9e48365fix doctest inside it documentation ; wrap some long lines

comment:28 Changed 12 months ago by Matthias Köppe

Authors: Emmanuel Charpentier, Matthias KoeppeEmmanuel Charpentier, Matthias Koeppe, Frédéric Chapoton

comment:29 Changed 12 months ago by git

Commit: 9e483652d0fe72ca59c5af56daacf4c56955b50705fa319b65be9b31487bb493450510e32c2f0d07

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

05fa319some more details

comment:30 Changed 12 months ago by Frédéric Chapoton

ok, looks good. Patchbot is morally green.

I vote for positive review. Any confirmation from other reviewers or authors ?

comment:31 in reply to:  30 ; Changed 12 months ago by Emmanuel Charpentier

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 not-so-easy to integrate). Stay tuned...

comment:32 in reply to:  31 Changed 12 months ago by Emmanuel Charpentier

Status: needs_reviewpositive_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 not-so-easy 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 Volker Braun

Branch: public/ticket/3179605fa319b65be9b31487bb493450510e32c2f0d07
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.