make sage variables unique in maxima

[jasongrout]

This patch prepends a unique string, "SAGE_VAR", to each variable name in maxima, to avoid conflicts with existing variables in maxima.

Depends on #6882

CC: @williamstein @sagetrac-acleone @kcrisman @robert-marik @burcin @saliola

Component: interfaces

Author: Jason Grout, Ralf Stephan

Branch/Commit: 7a6696b

Reviewer: Volker Braun, Paul Zimmermann, Karl-Dieter Crisman

Issue created by migration from https://trac.sagemath.org/ticket/8734

[jasongrout]

comment:1

Attachment: trac-8734-maxima-vars.patch.gz

I haven't run doctests on everything, but this patch is a start, if one of you wants to take it from here.

[kcrisman]

comment:2

I think there might also be another ticket about this somewhere...

[robert-marik]

comment:3

Hi, it is also #8634 and there is a dicussion at sage-devel

[jasongrout]

comment:5

This should fix #6882

[jasongrout]

comment:6

Replying to @jasongrout:

This should fix #6882

Well, or at least help with the solution, anyway.

[qed777]

comment:9

This ticket may also fix #9538.

[zimmermann6]

comment:11

patch should be rebased, it fails to apply to Sage 5.11:

sage: hg_sage.import_patch("/tmp/trac-8734-maxima-vars.patch")
cd "/home/zimmerma/Downloads/sage-5.11/devel/sage" && sage --hg import   "/tmp/trac-8734-maxima-vars.patch"
applying /tmp/trac-8734-maxima-vars.patch
patching file sage/calculus/calculus.py
Hunk #1 FAILED at 1450
Hunk #2 FAILED at 1461
2 out of 2 hunks FAILED -- saving rejects to file sage/calculus/calculus.py.rej
patching file sage/symbolic/assumptions.py
Hunk #1 FAILED at 100
1 out of 1 hunks FAILED -- saving rejects to file sage/symbolic/assumptions.py.rej
abort: patch failed to apply

Paul

[zimmermann6]

Work Issues: rebase

[rwst]

Branch: u/rws/ticket/8734

[rwst]

Reviewer: Ralf Stephan

[rwst]

comment:14

Rebased on 6.2.beta3

Patch doesn't seem ready:

sage -t --long src/sage/symbolic/integration/integral.py  # 3 doctests failed
sage -t --long src/sage/symbolic/assumptions.py  # 26 doctests failed
sage -t --long src/sage/symbolic/pynac.pyx  # 1 doctest failed
sage -t --long src/sage/symbolic/expression.pyx  # 21 doctests failed
sage -t --long src/sage/calculus/desolvers.py  # 63 doctests failed
sage -t --long src/sage/calculus/calculus.py  # 13 doctests failed
sage -t --long src/sage/calculus/functional.py  # 1 doctest failed

---

New commits:

87d944f

Trac #8734: Make sage variables unique in maxima.

[rwst]

Commit: 87d944f

[sagetrac-git]

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

5b76872	Merge branch 'develop' into needswork/8734
ca6a221	factor out missing assumption error handling; filter _SAGE_VAR_
3b7e916	fix typo leading to error
0ee7f20	two further maxima calls adapted; two more doctests fixed

[sagetrac-git]

Changed commit from 87d944f to 0ee7f20

[rwst]

comment:16

This fixes the errors in symbolic. More to come...

[rwst]

Changed work issues from rebase to fix doctest problems

[rwst]

Changed reviewer from Ralf Stephan to none

[rwst]

Changed author from Jason Grout to Jason Grout, Ralf Stephan

[kcrisman]

comment:52

Again, for me, all is positive review except the diffeq commit, and that only because I haven't had time to look carefully at it, not because I suspect any problems - it looks quite thorough.

[rwst]

Changed dependencies from #15530 to none

[kcrisman]

comment:54

Okay, I have a few questions about the ode changes. I assume the answer to all of them is "it would have been even messier any other way", but I just wanted to check.

• In some of the sanitizing functions, you replace things like '_SAGE_VAR_f with f, but in others you only replace the independent variable that way. Is that because of specific examples that didn't work, or was the context different, or... ?
• I'm wondering whether the Sage translation would have just taken care of this in soln.sage(), but I guess it didn't. Was there any possible change to the translation that could have done this, rather than getting into the ode wrapper code directly (which makes it harder to read)? For instance, in desolve_laplace we convert the de to Maxima (presumably adding SAGE_VAR, add another SAGE_VAR from f(x) to f(_SAGE_VAR_x) (I think), and then proceed to remove only the SAGE_VAR from the dependent variable. So... that part isn't taken care of by the translation, but the independent variable still somehow is translated back correctly, but not forward within de0=de._maxima_()? Yet in the rk4 types this isn't a problem, apparently.
• We should probably just remove desolve_system_strings, see fix documentation related to ODE solvers #8132 where it was first said to be obsolete, and it hasn't been in the global namespace since before 2010. That is pretty much equivalent to a deprecation to me. However, we should keep any non-overlapping examples - so maybe removal should be another ticket...
  But it seems good, assuming I didn't miss any tests that fail...

[kcrisman]

comment:55

But it seems good, assuming I didn't miss any tests that fail...

I didn't. So as long as you give the answers I expect, we are all set here.

[rwst]

comment:56

Working from the bottom up ...

Replying to @kcrisman:

• We should probably just remove desolve_system_strings, see fix documentation related to ODE solvers #8132 where it was first said to be obsolete, and it hasn't been in the global namespace since before 2010. That is pretty much equivalent to a deprecation to me. However, we should keep any non-overlapping examples - so maybe removal should be another ticket...

This is now #16568.

[sagetrac-git]

Changed commit from 793bb05 to a195985

[sagetrac-git]

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

a195985

8734: remove superfluous code

[rwst]

comment:58

Replying to @kcrisman:

• In some of the sanitizing functions, you replace things like '_SAGE_VAR_f with f, but in others you only replace the independent variable that way. Is that because of specific examples that didn't work, or was the context different, or... ?

It certainly was then because of specific examples. The removal of the code I think you mean makes no difference now, however.

• I'm wondering whether the Sage translation would have just taken care of this in soln.sage(), but I guess it didn't. Was there any possible change to the translation that could have done this, rather than getting into the ode wrapper code directly (which makes it harder to read)? For instance, in desolve_laplace we convert the de to Maxima (presumably adding SAGE_VAR, add another SAGE_VAR from f(x) to f(_SAGE_VAR_x) (I think), and then proceed to remove only the SAGE_VAR from the dependent variable. So... that part isn't taken care of by the translation, but the independent variable still somehow is translated back correctly, but not forward within de0=de._maxima_()? Yet in the rk4 types this isn't a problem, apparently.

With the above removal of superfluous code I don't see any different behaviour in the three functions laplace/system/rk4. There is always the marking of the dependent var to prepare for the call to solve. You cannot remove this because maxima(cmd) does no translation, it's the low-level call. The translation has to be done here, as far as I understand.

[kcrisman]

comment:59

With the above removal of superfluous code I don't see any different behaviour in the three functions laplace/system/rk4. There is always the marking of the dependent var to prepare for the call to solve. You cannot remove this because maxima(cmd) does no translation, it's the low-level call. The translation has to be done here, as far as I understand.

Again, I'm just confused because in laplace/rk4 the dependent variable gets _SAGE_VAR_ but in desolve it doesn't. Maybe this isn't important, though. And is the last change in desolve ok because putting things into Maxima takes care of the _SAGE_VAR_ already?

[kcrisman]

comment:60

Passes all tests.

[rwst]

comment:61

After studying this in detail, the reason why the last change makes no difference is the following: in desolve and desolve_laplace the translation to Maxima is applied several times using maxima() and P() (which is the parent of the first translation result, i.e. of a Maxima expression). P() is also applied to dvar.operator() resulting in dvar_str which already has _SAGE_VAR_ and is template for sanitization. Appending _SAGE_VAR_ to dvar_str and replacing it thus was useless.

In desolve_rk4 the cmd is constructed from two parts: the de0 that gets translated via ._maxima()_ and the construction via string concatenation. So, there is no difference between desolve and desolve_rk4 because desolve gets _SAGE_VAR_ everywhere and desolve_rk4 too.

[kcrisman]

comment:62

Let's make it so, then. I was kind of suspecting there were extra _SAGE_VAR_s.

[vbraun]

comment:63

conflicts with #6882

[sagetrac-git]

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

65107d4	Merge branch 'develop' into t/8734/ticket/8734-1
4e07383	6882: add rules for e, i, I
e5a5343	6882: do word search/replace for symtable keys
4c1b0eb	6882: correction to previous commit
518de3e	6882: add symable rules for e,i,I; fix maxima_var; add doctests
7a6696b	Merge branch 'u/rws/bugs_in_conversion_of_variable_names_from_maxima_to_sage' of trac.sagemath.org:sage into t/8734/ticket/8734-1

[sagetrac-git]

Changed commit from a195985 to 7a6696b

[rwst]

comment:65

Since the reviewed #6882 is orthogonal I set positive again. Thanks Karl-Dieter for this review! I would be happy if you could have a look at #2516 too.

[rwst]

Dependencies: #6882

[zimmermann6]

Changed reviewer from Volker Braun, Paul Zimmerman, Karl-Dieter Crisman to Volker Braun, Paul Zimmermann, Karl-Dieter Crisman

[kcrisman]

comment:67

I would be happy if you could have a look at #2516 too.

Sorry, that one I definitely like the idea of and have reviewed many similar ones in the past, but just don't have the time currently to review much new functionality carefully. :(

[vbraun]

Changed branch from u/rws/ticket/8734-1 to 7a6696b