sagemath / sage

Main repository of SageMath. Now open for Issues and Pull Requests.
https://www.sagemath.org
Other
1.19k stars 411 forks source link

conversion from maxima buggy #16898

Closed rwst closed 9 years ago

rwst commented 9 years ago
On Wed, Aug 27, 2014 at 2:46 PM, Peter Mueller <ypf...@googlemail.com> wrote: 
> My understanding of Sage is that var('e') declares e as a symbolic variable, 
> no matter that it was the Euler number before. The last line leaves me 
> clueless what goes wrong ... 
> 
> sage: var('e') 
> e 
> sage: matrix.diagonal([e,1,1]).det() 
> e 
> sage: matrix.diagonal([e,1,1,1]).det() 
> _e 

On Wednesday, August 27, 2014 3:13:58 PM UTC+2, Stein William wrote:
The real bug is in conversion from Maxima to Sage.  Observe: 

~/wstein/sage-6.4.beta1$ ./sage 
sage: maxima('_SAGE_VAR_e')._sage_() 
_e 

The same, as suspected, with i, so #6882 is the culprit.

CC: @kcrisman

Component: interfaces

Keywords: maxima, conversion, variable

Author: Nils Bruin

Branch: ed8d3b1

Reviewer: Ralf Stephan

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

rwst commented 9 years ago
comment:1

Well, so sefms should know about existing Sage variables and prepend the underscore only if the variable doesn't exist:

sage: sefms('e')
_e
sage: e = var('e')
sage: sefms('e')
e

To recall, some marking is needed because removal of _SAGE_VAR_ has already happened before sefms, and e could well be a Maxima variable. We just didn't think that someone could use such Sage variables.

kcrisman commented 9 years ago
comment:3

Aargh!!! But checking variables is often a nightmare, and there are other things that could be used for e too.

rwst commented 9 years ago
comment:4

But the other things are not shoved to Maxima and back, are they?

kcrisman commented 9 years ago
comment:5

Well, anything in a symbolic matrix (or a symbolic integral, or limit) gets sent to Maxima and back. But maybe just raw variables are the only worry here.

The following would be the thing to fix.

sage: e = var('e')
sage: e == maxima(e)._sage_()
e == _e

whereas

sage: reset()
sage: e == maxima(e)._sage_()
e == e
rwst commented 9 years ago
comment:6

Replying to @kcrisman:

The following would be the thing to fix.

sage: e = var('e')
sage: e == maxima(e)._sage_()
e == _e

whereas

sage: reset()
sage: e == maxima(e)._sage_()
e == e

I don't think so. It should both be e == e because the first time you're giving the variable as argument, and the second time the constant. Rather:

sage: e = var('e')
sage: e == maxima(e)._sage_()
e == e
sage: reset()
sage: e == maxima(e)._sage_()
e == e
sage: e == maxima('e')._sage_()
e == _e

Fix proposal following.

rwst commented 9 years ago

Branch: u/rws/conversion_from_maxima_buggy

rwst commented 9 years ago
comment:8

It was much easier than feared because we could get to the string before removal of _SAGE_VAR_. The method is to mark specific maxima vars in the string with underscore. Note: this or a different prefix could be given to all maxima vars.


New commits:

f977bad16898: do not give sage vars the pre-underscore
rwst commented 9 years ago

Commit: f977bad

kcrisman commented 9 years ago
comment:9

The following would be the thing to fix.

I don't think so. It should both be e == e because the first time you're giving the variable as argument, and the second time the constant. Rather:

I was actually agreeing with you - it was the thing to FIX. :-)

Fix proposal following.

Okay, I'll look at this.

kcrisman commented 9 years ago
comment:10
s[:m.start()-1] + '_' + s[m.start()]

should that maybe just be

s[:m.start()] + '_' + s[m.start()]

we're not getting rid of anything, are we? Just putting in an underscore? The documentation seems to suggest this, since Python slices have "the end always excluded". I don't think any of your examples catch that, since they just have e. But foo+bar+e might get turned to foo+bar_e? (Sorry for not trying this out quite yet, I don't have an easy-access branch just now.)

nbruin commented 9 years ago
comment:11

Would it be possible to fix this properly? It should really be easier to do the conversion properly now that variables get converted to something with a _SAGE_VAR_ prefix.

From sage to maxima: At this point you know whether an object is a symbolic variable or a (predefined) symbolic constant. In one case you convert it to the string _SAGE_VAR_e, in the other case you convert it to %e.

From maxima to sage: This is the trickier one, because the strings-based interface needs to try to tell everything from string representations. However, since we're either seeing %e or _SAGE_VAR_e, there's not really any confusion. Whether a variable "e" exists doesn't even matter. Indeed, we have

sage: var('e')
e
sage: e==exp(1)
e == e
sage: bool(e==exp(1))
False
sage: (e==exp(1))._maxima_init_()
'_SAGE_VAR_e = exp(1)'

so we should make sure that %e gets converted back to sage.functions.log.exp(1) and that that _SAGE_VAR_e gets converted to SR.var('e').

The round trip currently goes horribly wrong:

sage: A=e==exp(1)
sage: A
sage: maxima_calculus(A) # this is fine
_SAGE_VAR_e=%e
sage: A_roundtrip=maxima_calculus(A)._sage_()
sage: A_roundtrip.lhs() #a new symbol got invented!
_e
sage: bool(A_roundtrip.rhs() == exp(1)) #and the right hand side is now a symbolic variable!
False
sage: bool(maxima_calculus(A)._sage_())
False
sage: bool(maxima_calculus(maxima_calculus(A)._sage_())._sage_()) #fun all the way.
True

If confusion arises, then this is probably due to inappropriate string substitutions. It may well be that sefms is up for a major refactoring.

I was a little surprised that max_to_sr and sr_to_max get this wrong too:

sage: max_to_sr(maxima_calculus(sr_to_max(A)).ecl())
e == e
sage: bool(max_to_sr(maxima_calculus(sr_to_max(A)).ecl()))
True

but this may be because currently, sr_to_max/max_to_sr get their initial translations from string-based. Certainly the infrastructure for these routines fully allows to distinguish objects regardless of their printed representation. These could be fixed by handling "symbolic variables" and "_SAGEVAR..." symbols (on the lisp side) separately (in sr_to_max/max_to_sr). That would reduce chances that we mess up the translation tables. In fact, objects of type sage.symbolic.function_factory.NewSymbolicFunction should probably be translated to _SAGE_FUNC_... in maxima for similar reasons.

nbruin commented 9 years ago
comment:12

perhaps check out _find_var and _find_func, used in symbolic_expression_from_string (in the same file) rather than symbolic_expression_from_maxima_string. Those work on string tokens once the expression is actually parsed. Conversion there should be much more straightforward than by string-based preprocessing using regular expressions, which is what happens in symbolic_expression_from_maxima_string. Probably, anything we can do there rather than by dumb string manipulations will be much better and easier to maintain than the mess we have now. Certainly, making _find_var aware of _SAGE_VAR_... rather than just stripping out this valuable marker will lead to a much more efficient and reliable conversion process. I don't have the time to dive into the peculiarities of the relevant routines, but the code looks pretty straightforward, so hopefully someone is willing to invest a little time in refactoring this code. It will make working on strings-based maxima-to-sage conversions much more pleasurable.

rwst commented 9 years ago
comment:13

Well, I think it's better to apply this quick fix and work towards abandoning of the expect interface.

nbruin commented 9 years ago
comment:14

Replying to @rwst:

Well, I think it's better to apply this quick fix and work towards abandoning of the expect interface.

The proposed fix doesn't solve the issue, though. I've tried the branch here and I get this:

sage: A = SR.var('e')==exp(1)
sage: A # the printing of this may be confusing but the meaning to sage is clear
e == e
sage: bool(A)
False
sage: bool(SR(maxima_calculus(A))) #the distinction doesn't survive the round-trip.
True
sage: A.rhs().is_symbol()
False
sage: SR(maxima_calculus(A)).rhs().is_symbol()
True

so it seems that the existence of a variable e in pynac causes maxima's %e to be translated to SR.var('e'). Illustrating this directly:

sage: from sage.calculus.calculus import symbolic_expression_from_maxima_string as sefms
sage: sefms('%e').is_symbol()
False
sage: SR.var('e') #apparently this affects the translation
e
sage: sefms('%e').is_symbol()
True

It seems that changing the entry for symtable['%e'] to 'exp(1)' sort-of fixes this, but obviously, for %i and %I we have the same problem.

This won't be quite bulletproof either, due to:

sage: function('log')
log
sage: sage.functions.log.log(x) == log(x)

but that currently won't even make it to maxima (that would need a _SAGE_FUNCTION_log encoding)

nbruin commented 9 years ago

Changed branch from u/rws/conversion_from_maxima_buggy to u/nbruin/conversion_from_maxima_buggy

nbruin commented 9 years ago
comment:15

New branch, based on doing more at parser level rather than at string mangling level. Previous branch was:

u/rws/conversion_from_maxima_buggy

I think the present branch is already more in the direction. Important obstacle to using Bradshaw's parser straight on maxima output: It can't handle % characters as part of identifiers (it's an operator in python after all!), I think the current branch solves at least the problem stated in the ticket without creating new problems.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

e6cd65btrac #16898: Use parser to distinguish between maxima internal and sage variable names
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from f977bad to e6cd65b

rwst commented 9 years ago
comment:17

Buildbot reports some order changes in doctests of src/sage/matrix/matrix2.pyx. Also, I'm not sure if it fits the ticket, I found the following:

sage: pari.pollegendre(4,e)
35/8*e^4 - 15/4*e^2 + 3/8
sage: SR(_)
35/8*e^4 - 15/4*e^2 + 3/8
sage: _.simplify_full()
35/8*_e^4 - 15/4*_e^2 + 3/8
nbruin commented 9 years ago
comment:18

Replying to @rwst:

sage: pari.pollegendre(4,e)
35/8*e^4 - 15/4*e^2 + 3/8
sage: SR(_)
35/8*e^4 - 15/4*e^2 + 3/8
sage: _.simplify_full()
35/8*_e^4 - 15/4*_e^2 + 3/8

Nice one. That's a separate ticket, I think, though:

sage: f=pari.pollegendre(4,e)
sage: g=SR(f)
sage: g.operator()
sage: g.operands()
[]
sage: g.pyobject() is f
True

Apparently pari "polynomials" don't get properly converted to SR, but just get stuffed in. Consequently, the sage-to-maxima conversion just sees if maxima can make sense of the string representation. For instance (and that's what you see) variables don't get properly converted:

sage: maxima_calculus(g) #note no _SAGE_VAR_ prefixes
35*e^4/8-15*e^2/4+3/8

Other paths lead to errors:

sage: QQ['e'](g)
TypeError: Unable to coerce PARI 35/8*e^4 - 15/4*e^2 + 3/8 to an Integer

The truth is, almost everything can be stuffed in SR and, as a result, not everything in SR can be translated to maxima:

sage: M=pari.matrix(2,2)
sage: M
[0, 0; 0, 0]
sage: M.simplify_full()
AttributeError: 'MaximaLibElement' object has no attribute '_name'
sage: maxima_calculus(M) #maxima's reader chokes on [0,0;0,0] because it's ungrammatical in maximan
TypeError: ECL says: THROW: The catch MACSYMA-QUIT is undefined.

We can hide the bad behaviour in the particular example you gave by folding both _SAGE_VAR_e and e back onto e but you'd still get wrong answers:

sage: integrate(g,e)
integrate(35/8*e^4 - 15/4*e^2 + 3/8, e)
sage: integrate(g,e).simplify()
1/8*(35*_e^4 - 30*_e^2 + 3)*e

In the latter one, at least we now see something funny has happened. If it were to multiply out, it's truly confusing.

jdemeyer commented 9 years ago
comment:19

This was reported again at #17187.

kcrisman commented 9 years ago
comment:20

Thanks, Jeroen. So what needs fixing here is just some doctests?

rwst commented 9 years ago

Changed branch from u/nbruin/conversion_from_maxima_buggy to public/16898_conversion_from_maxima_buggy

rwst commented 9 years ago

Reviewer: Ralf Stephan

rwst commented 9 years ago

Changed commit from e6cd65b to ed8d3b1

rwst commented 9 years ago

New commits:

c38f654Merge branch 'develop' into t/16898/conversion_from_maxima_buggy
ed8d3b116898: fix failing doctests in matrix2
rwst commented 9 years ago

Author: Nils Bruin

nbruin commented 9 years ago
comment:23

I'm OK with the amended doctests, so if someone else is happy with the other changes, this ticket can be set to positive review.

rwst commented 9 years ago
comment:24

Replying to @nbruin:

I'm OK with the amended doctests, so if someone else is happy with the other changes, this ticket can be set to positive review.

Completely forgot about this. Your part is fine, and patchbot is happy.

vbraun commented 9 years ago

Changed branch from public/16898_conversion_from_maxima_buggy to ed8d3b1

kcrisman commented 9 years ago

Changed commit from ed8d3b1 to none

kcrisman commented 9 years ago
comment:26

See http://ask.sagemath.org/question/26489/compute-the-determinant-of-a-symbolic-5x5-matrix/ though I assume that is exactly what is fixed here.