Opened 13 years ago

Closed 8 years ago

#6882 closed defect (fixed)

bugs in conversion of variable names from Maxima to Sage

Reported by: William Stein Owned by: Ralf Stephan
Priority: major Milestone: sage-6.3
Component: calculus Keywords:
Cc: Robert Marik Merged in:
Authors: Ralf Stephan Reviewers: Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: 518de3e (Commits, GitHub, GitLab) Commit: 518de3e62533cd209997107f903192f1a31d118c
Dependencies: Stopgaps:

Status badges

Description (last modified by Ralf Stephan)

-----------
sage: from sage.calculus.calculus import symbolic_expression_from_maxima_string
sage: symbolic_expression_from_maxima_string('%i')
I
sage: symbolic_expression_from_maxima_string('i')
I
sage: symbolic_expression_from_maxima_string('%inf')
Inf
-----------

So as you see, we are converting both '%i' and 'i' to  imaginary 'I' !!!!

The ticket should implement a multi word replace and use that on a symtable with additional entries 'e':'_e', 'i':'_i', 'I':'_I'.

We do not want to be surprised when some new Maxima variable starting %i is introduced. At the moment it's really just a string replace from %i to I, without sense of word boundaries.

Change History (40)

comment:1 Changed 13 years ago by Karl-Dieter Crisman

Here is the relevant discussion.

comment:2 Changed 13 years ago by Karl-Dieter Crisman

This will also happen with %e and e, and any other similar pairs, so a fix should take care of all of them.

comment:3 Changed 13 years ago by Robert Marik

Report Upstream: N/A

As another consequence, solving of ode y'=c*x is not correct, the free variable is messed up with a parameter, see sage-devel - thanks for Yuri Karadzhov

[marik@um-bc107 /opt/sage-4.3.4]$ ./sage
----------------------------------------------------------------------
| Sage Version 4.3.4, Release Date: 2010-03-19                       |
| Type notebook() for the GUI, and license() for information.        |
----------------------------------------------------------------------
sage: from sage.calculus.calculus import symbolic_expression_from_maxima_string
sage: symbolic_expression_from_maxima_string('%c')
c
sage: c=var('c'); y=function('y',x); eq=diff(y,x)==c*x; eq
D[0](y)(x) == c*x
sage: desolve(eq,y,ivar=x)
1/2*c*x^2 + c

the answer should be something like 1/2*c*x2 + c1

comment:4 Changed 13 years ago by Jason Grout

See #8734 for what I think is a "needs work" solution.

comment:5 Changed 13 years ago by Jason Grout

Actually, I guess the patch at #8734 will *help* with the solution, but may not totally solve the problem.

comment:6 Changed 12 years ago by Robert Marik

Cc: Robert Marik added

This should fix also #9421.

comment:7 Changed 10 years ago by Karl-Dieter Crisman

Also, in a situation where we don't have the duplication of constants, we get

sage: c
Traceback (click to the left of this block for traceback)
...
NameError: name 'c' is not defined

which isn't good either, though apparently that part of the expression still has type SymbolicExpression.

comment:8 Changed 9 years ago by Jeroen Demeyer

Milestone: sage-5.11sage-5.12

comment:9 Changed 9 years ago by For batch modifications

Milestone: sage-6.1sage-6.2

comment:10 Changed 9 years ago by Ralf Stephan

Priority: majorcritical

Set to critical due to dependent tickets.

comment:11 Changed 9 years ago by Ralf Stephan

Last edited 9 years ago by Ralf Stephan (previous) (diff)

comment:12 in reply to:  5 Changed 9 years ago by Ralf Stephan

Replying to jason:

Actually, I guess the patch at #8734 will *help* with the solution, but may not totally solve the problem.

Indeed, because the patch at #8734 (needing review) only is about vars, and it will only help with the problem in comment:3 if the then marked sage vars are renamed to some other specific string before output in desolve...().

comment:13 Changed 9 years ago by Ralf Stephan

Dependencies: #8734

comment:14 in reply to:  3 Changed 9 years ago by Ralf Stephan

Dependencies: #8734#8734, #16007

Replying to robert.marik:

As another consequence, solving of ode y'=c*x is not correct ... the answer should be something like 1/2*c*x2 + c1

This one is resolved in #16007. So it seems only variables are left (#8734).

comment:15 Changed 9 years ago by Karl-Dieter Crisman

Yes, variables are all that's left, but the other way around! (Don't forget the initial examples of this ticket.) We need to disambiguate Maxima variables like i and e from the things that become those in Sage - %i and %e. I suppose one could take the Maxima variables i and I and turn them into _i and _I, and likewise for e, as at #16007, but I'm not sure if that's ideal or not. Thoughts?

Last edited 9 years ago by Karl-Dieter Crisman (previous) (diff)

comment:16 Changed 9 years ago by For batch modifications

Milestone: sage-6.2sage-6.3

comment:17 Changed 9 years ago by Ralf Stephan

Priority: criticalminor

Priority changed as the more important fixes were outsourced to other tickets.

comment:18 in reply to:  17 Changed 9 years ago by Karl-Dieter Crisman

Priority changed as the more important fixes were outsourced to other tickets.

Hmm, though the BDFL originally reported this with the comment

I think my email must have not been clear.  I think it's an instance   
of a *HUGE BUG* in Sage.  No more, no less.    

comment:19 Changed 8 years ago by Karl-Dieter Crisman

Priority: minormajor

Maybe we can change the Maxima i and e to Sage _i and _e, leaving %i and %e to become i and e as currently, and then taking advantage of the last changes at #16007 for typesetting more-or-less properly.

comment:20 Changed 8 years ago by Ralf Stephan

Status: newneeds_info

The original bug report on sage-devel had:

sage: var('i')
i
sage: i
i
sage: a = i^2
sage: a.simplify_full()
-1

However, with develop I get i^2. Is this ticket still valid?

comment:21 Changed 8 years ago by Karl-Dieter Crisman

And indeed,

sage: from sage.calculus.calculus import symbolic_expression_from_maxima_string
sage: symbolic_expression_from_maxima_string('i')
i
sage: symbolic_expression_from_maxima_string('%i')
I

And the original solving case William reported is also fixed.

Huh. Is this before or after #8734? (I would imagine that one would have an impact.) Anyway, I would say we add some doctests (for both the sefms and i^2 cases) and call it a day, regardless of which ticket it depends on. Good work, since ideally one wouldn't be creating a Maxima i and then trying to bring it to Sage.

comment:22 Changed 8 years ago by Ralf Stephan

Status: needs_infoneeds_work

With develop:

sage: from sage.calculus.calculus import symbolic_expression_from_maxima_string
sage: symbolic_expression_from_maxima_string('t')
t
sage: symbolic_expression_from_maxima_string('i')
I
sage: var('i')
i
sage: symbolic_expression_from_maxima_string('i')
i

So that example is a different animal than the ticket case.

comment:23 Changed 8 years ago by Karl-Dieter Crisman

Okay, I see. But the thing is that, in principle, we should never get i inside Maxima without asking for it via Sage having a variable i; we should only get %i the imaginary in Maxima, which translates differently (and correctly) now. Does that make sense, or do you think this is still worth fixing? (We should check what happens with e as well, maybe via ln(e).)

comment:24 Changed 8 years ago by Ralf Stephan

With #8734 I can result from a Sage I variable, from Maxima %i, or from Maxima i which is only creatable from Sage via the Maxima interface. But not from any trick involving the Sage variable i due to the protection via _SAGE_VAR_. The i case is why Jason said #8734 will help, but not totally solve the problem.

I don't know why the i^2 example failed at all, and when exactly it stopped failing. Maybe it didn't fail; certainly nobody posted a confirmation message. And certainly we all confirmed the sefms snippet because that's what the ticket presented.

comment:25 Changed 8 years ago by Karl-Dieter Crisman

Yeah, even in Sage 4.4.4 (which I have lying around due to some custom fixes I use for research I'm too lazy to update)

sage: var('i')
i
sage: i
i
sage: a = i^2
sage: a
i^2
sage: a.simplify_full()
i^2

Your thoughts on a resolution? The thing that was a bug is not there any more, and the only potential bug is from 'user error', in some sense.

comment:26 Changed 8 years ago by Ralf Stephan

Description: modified (diff)
Owner: changed from Burcin Erocal to Ralf Stephan
Summary: bug in conversion of "i" from Maxima to Sagebugs in conversion of variable names from Maxima to Sage

At the moment we also get behaviour like

sage: symbolic_expression_from_maxima_string('%inf')
Inf

so I think the ticket should implement multi_word_replace() in sage.misc.multireplace and use that on a symtable with additional entries 'e':'_e', 'i':'_i', 'I':'_I'.

comment:27 in reply to:  26 ; Changed 8 years ago by Karl-Dieter Crisman

At the moment we also get behaviour like

sage: symbolic_expression_from_maxima_string('%inf')
Inf

Is %inf a normal Maxima expression, though? They just use inf and minf, I believe, which we replace correctly.

so I think the ticket should implement multi_word_replace() in sage.misc.multireplace and use that on a symtable with additional entries 'e':'_e', 'i':'_i', 'I':'_I'.

I guess one could do so... I'm just trying to imagine cases in which this would be necessary due only to Sage usage. If someone uses Maxima to create variables it's quite different.

comment:28 in reply to:  27 ; Changed 8 years ago by Ralf Stephan

Replying to kcrisman:

Is %inf a normal Maxima expression, though? They just use inf and minf, I believe, which we replace correctly.

No but we do not want to be surprised when some new Maxima variable starting %i is introduced. At the moment it's really just a string replace from %i to I, without sense of word boundaries.

comment:29 Changed 8 years ago by Ralf Stephan

Description: modified (diff)

comment:30 in reply to:  28 Changed 8 years ago by Karl-Dieter Crisman

Is %inf a normal Maxima expression, though? They just use inf and minf, I believe, which we replace correctly.

No but we do not want to be surprised when some new Maxima variable starting %i is introduced. At the moment it's really just a string replace from %i to I, without sense of word boundaries.

Aha! I didn't catch that was the reason. I don't think Maxima introduces many new constants with % but I see your point.

comment:31 Changed 8 years ago by Ralf Stephan

Branch: u/rws/bugs_in_conversion_of_variable_names_from_maxima_to_sage

comment:32 Changed 8 years ago by Ralf Stephan

Authors: Ralf Stephan
Commit: 518de3e62533cd209997107f903192f1a31d118c
Dependencies: #8734, #16007
Status: needs_workneeds_review

I also found an error in the definition for maxima variable, because it didn't allow variable names without '%' or the '%' not at the beginning. Now the mentioned rules can be expressed as simple entries in symtable.


New commits:

4e073836882: add rules for e, i, I
e5a53436882: do word search/replace for symtable keys
4c1b0eb6882: correction to previous commit
518de3e6882: add symable rules for e,i,I; fix maxima_var; add doctests

comment:33 Changed 8 years ago by Ralf Stephan

Description: modified (diff)

comment:34 in reply to:  32 ; Changed 8 years ago by Karl-Dieter Crisman

I also found an error in the definition for maxima variable, because it didn't allow variable names without '%' or the '%' not at the beginning. Now the mentioned rules can be expressed as simple entries in symtable.

You'll note that maxima_var was never currently used in the codebase, so it wasn't a problem, more of just old code - here is where the % were all replaced. If you wanted I guess you could just replace that with _ and it would be much more efficient than doing this whole loop every time, or so it seems to me. How does this do with timeit for long expressions one gets in 'real life'? (See sage/symbolic/random_tests.py.) Also note that usually we end up having to special-case things with % anyway - e.g., %ilt (inverse Laplace transform) gets handled somewhere else, I'd have to look up where.

comment:35 in reply to:  34 ; Changed 8 years ago by Ralf Stephan

Replying to kcrisman:

How does this do with timeit for long expressions one gets in 'real life'? (See sage/symbolic/random_tests.py.)

It's within the measurement error:

sage: from sage.calculus.calculus import symbolic_expression_from_maxima_string as sefms
sage: var('v1,v2,v3')
(v1, v2, v3)
sage: ex=-1/3*(pi + 1)*(3*(euler_gamma - e)*(pi - 3*v1 - v1/arcsech(1) + e^(-1)/pi) - 6*v3^2*arcsinh(-v1 + e)/v2 - v2 - 3*log_gamma(v1*v3)/v2 - 3*e^(-254) + 3)*(-catalan/v3)^(twinprime*pi - 1/2*v1 - 1/2*v2)*inverse_jacobi_cs(1, v3)/jacobi_sc(1/arccos(-1/(v1*csc(v3))), v3/v1 + e) - 1/4*(2*v3^2*(e + 1) + ((e*log_integral(arcsech(exp_integral_e1(v2^mertens - 1) - 4)) + 15*v1 + jacobi_dn(v2, pi))*v1*e^(-1) + golden_ratio*pi^v1*(1/v3^12 + jacobi_ds(-10, csc(v3^2)))^(v2 - 1/2/v2)*sinh(v2*e)/((v1 + v2 + v3 + 1)*v2))/(v1^2*inverse_jacobi_nc(v1, -e)) - 2*bessel_Y(v3, v2))/v2 + v3/inverse_jacobi_sc(1, v2) - (v1 + 1)/((v2 + v3)*(2*(v1 + e)*(v3 - 1)/(pi + v1) + (-v3*sech(v1*v3)/v2)^(-e/v1))) + inverse_jacobi_cn(pi + v1*v3, pi - v3) + jacobi_sn(e, arctanh(-(-log_integral(2) + log_integral(jacobi_ds(-1, v3)))^v2*e)^(1/7*(18*v2 - v3)*(14*v2 + e)/(v3*arctan(1/v2)*kronecker_delta(v1, v3))))
sage: timeit('sefms(str(ex))')
5 loops, best of 3: 2.19/2.14/2.15 s per loop (without #6882)
5 loops, best of 3: 2.13/2.11/2.12 s per loop (with #6882)

Obviously the whole routine takes so long that the loop doesn't signify. That may be worth an optimization ticket alone.

Also note that usually we end up having to special-case things with % anyway - e.g., %ilt (inverse Laplace transform) gets handled somewhere else, I'd have to look up where.

Just put it in symtable, give it a special value, and ask for that value within the new loop. Having it all in the loop is more efficient than find every time.

comment:36 in reply to:  35 ; Changed 8 years ago by Ralf Stephan

Replying to rws:

sage: timeit('sefms(str(ex))')
5 loops, best of 3: 2.19/2.14/2.15 s per loop (without #6882)
5 loops, best of 3: 2.13/2.11/2.12 s per loop (with #6882)

It calls 120 times the function MaximaLib._eval_line() which takes 17ms in average = 2 seconds. 17 ms is an eternity.

comment:37 in reply to:  36 Changed 8 years ago by Karl-Dieter Crisman

sage: timeit('sefms(str(ex))')
5 loops, best of 3: 2.19/2.14/2.15 s per loop (without #6882)
5 loops, best of 3: 2.13/2.11/2.12 s per loop (with #6882)

Wow, those timings are indeed an eternity, though presumably not for shorter expressions. I'll put this ticket in my "finally finish reviewing" queue, then, for sure.

comment:38 Changed 8 years ago by Karl-Dieter Crisman

Reviewers: Karl-Dieter Crisman

Well, it's an improvement, though still not perfect:

# Before
sage: sefms('%inf')
Inf
sage: sefms('%minf')
-Infinity
# After
sage: sefms('%inf')
+Infinity
sage: sefms('%minf')
-Infinity

Neither of these are infinity in Maxima, of course. And indeed here is what the Maxima manual says about identifiers:

(%i1) %an_ordinary_identifier42;
(%o1)               %an_ordinary_identifier42

"A numeral may be the first character of an identifier if it is preceded by a backslash. " But I don't know that we need to translate all identifiers in Maxima to Sage here... I guess I'm torn about that.

sage: timeit('sefms(str(ex))')
5 loops, best of 3: 2.19/2.14/2.15 s per loop (without #6882)
5 loops, best of 3: 2.13/2.11/2.12 s per loop (with #6882)

Of course, thinking about it, that string is a Sage string, not a Maxima string, so %time R = random_expr(50,nvars=2); sefms(repr(R._maxima_())) is probably more accurate, but that is also wildly variant on the expression.

Okay, as far as I can tell this will not break anything (let's really hope!) and fixes the actual problem without slowing down what is already a very slow process (even for random_expr(5,nvars=2) it's 2-3 milliseconds either way). Step in right direction, and again most people should not be affected in the slightest. If passes tests, let's get it in.

comment:39 Changed 8 years ago by Karl-Dieter Crisman

Status: needs_reviewpositive_review

comment:40 Changed 8 years ago by Volker Braun

Branch: u/rws/bugs_in_conversion_of_variable_names_from_maxima_to_sage518de3e62533cd209997107f903192f1a31d118c
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.