Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#16898 closed defect (fixed)

conversion from maxima buggy

Reported by: rws Owned by:
Priority: major Milestone: sage-6.6
Component: interfaces Keywords: maxima, conversion, variable
Cc: kcrisman Merged in:
Authors: Nils Bruin Reviewers: Ralf Stephan
Report Upstream: N/A Work issues:
Branch: ed8d3b1 (Commits) Commit:
Dependencies: Stopgaps:

Description

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.

Change History (26)

comment:1 Changed 5 years ago by rws

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.

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

comment:2 Changed 5 years ago by rws

  • Cc kcrisman added

comment:3 Changed 5 years ago by kcrisman

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

comment:4 Changed 5 years ago by rws

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

comment:5 follow-up: Changed 5 years ago by kcrisman

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

comment:6 in reply to: ↑ 5 ; follow-up: Changed 5 years ago by rws

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.

comment:7 Changed 5 years ago by rws

  • Branch set to u/rws/conversion_from_maxima_buggy

comment:8 Changed 5 years ago by rws

  • Commit set to f977bad314fcce8bfb85d72564cc64545b71f1fb
  • Status changed from new to needs_review

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

comment:9 in reply to: ↑ 6 Changed 5 years ago by kcrisman

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.

comment:10 Changed 5 years ago by kcrisman

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.)

comment:11 Changed 5 years ago by nbruin

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 "_SAGE_VAR_..." 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.

comment:12 Changed 5 years ago by nbruin

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.

comment:13 follow-up: Changed 5 years ago by rws

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

comment:14 in reply to: ↑ 13 Changed 5 years ago by nbruin

Replying to rws:

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)

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

comment:15 Changed 5 years ago by nbruin

  • Branch changed from u/rws/conversion_from_maxima_buggy to u/nbruin/conversion_from_maxima_buggy

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.

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

comment:16 Changed 5 years ago by git

  • Commit changed from f977bad314fcce8bfb85d72564cc64545b71f1fb to e6cd65b2e11b88e57a19d993f75c9fec60af5c81

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

e6cd65btrac #16898: Use parser to distinguish between maxima internal and sage variable names

comment:17 follow-up: Changed 5 years ago by rws

  • Status changed from needs_review to needs_work

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

comment:18 in reply to: ↑ 17 Changed 5 years ago by nbruin

Replying to rws:

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.

comment:19 Changed 4 years ago by jdemeyer

This was reported again at #17187.

comment:20 Changed 4 years ago by kcrisman

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

comment:21 Changed 4 years ago by rws

  • Branch changed from u/nbruin/conversion_from_maxima_buggy to public/16898_conversion_from_maxima_buggy

comment:22 Changed 4 years ago by rws

  • Authors set to Nils Bruin
  • Commit changed from e6cd65b2e11b88e57a19d993f75c9fec60af5c81 to ed8d3b1cd0744940bbf9ac851c3f3a762ca54083
  • Reviewers set to Ralf Stephan
  • Status changed from needs_work to needs_review

New commits:

c38f654Merge branch 'develop' into t/16898/conversion_from_maxima_buggy
ed8d3b116898: fix failing doctests in matrix2

comment:23 follow-up: Changed 4 years ago by 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.

comment:24 in reply to: ↑ 23 Changed 4 years ago by rws

  • Milestone changed from sage-6.4 to sage-6.6
  • Status changed from needs_review to positive_review

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.

comment:25 Changed 4 years ago by vbraun

  • Branch changed from public/16898_conversion_from_maxima_buggy to ed8d3b1cd0744940bbf9ac851c3f3a762ca54083
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:26 Changed 4 years ago by kcrisman

  • Commit ed8d3b1cd0744940bbf9ac851c3f3a762ca54083 deleted
Note: See TracTickets for help on using tickets.