Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugs in conversion of variable names from Maxima to Sage #6882

Closed
williamstein opened this issue Sep 3, 2009 · 41 comments
Closed

bugs in conversion of variable names from Maxima to Sage #6882

williamstein opened this issue Sep 3, 2009 · 41 comments

Comments

@williamstein
Copy link
Contributor

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

CC: @robert-marik

Component: calculus

Author: Ralf Stephan

Branch/Commit: 518de3e

Reviewer: Karl-Dieter Crisman

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

@kcrisman
Copy link
Member

comment:1

Here is the relevant discussion.

@kcrisman
Copy link
Member

comment:2

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

@robert-marik
Copy link
Mannequin

robert-marik mannequin commented Mar 23, 2010

comment:3

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/2cx^2 + c1

@jasongrout
Copy link
Member

comment:4

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

@jasongrout
Copy link
Member

comment:5

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

@robert-marik
Copy link
Mannequin

robert-marik mannequin commented Jul 4, 2010

comment:6

This should fix also #9421.

@kcrisman
Copy link
Member

comment:7

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.

@jdemeyer jdemeyer modified the milestones: sage-5.11, sage-5.12 Aug 13, 2013
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014
@rwst
Copy link

rwst commented Feb 18, 2014

comment:10

Set to critical due to dependent tickets.

@rwst
Copy link

rwst commented Mar 24, 2014

comment:12

Replying to @jasongrout:

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

@rwst
Copy link

rwst commented Mar 24, 2014

Dependencies: #8734

@rwst
Copy link

rwst commented Mar 26, 2014

comment:14

Replying to @robert-marik:

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

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

@rwst
Copy link

rwst commented Mar 26, 2014

Changed dependencies from #8734 to #8734, #16007

@kcrisman
Copy link
Member

comment:15

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?

@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014
@rwst
Copy link

rwst commented Jun 6, 2014

comment:17

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

@kcrisman
Copy link
Member

comment:18

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.    

@rwst rwst assigned rwst and unassigned burcin Jun 27, 2014
@rwst rwst changed the title bug in conversion of "i" from Maxima to Sage bugs in conversion of variable names from Maxima to Sage Jun 27, 2014
@kcrisman
Copy link
Member

comment:27

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.

@rwst
Copy link

rwst commented Jun 27, 2014

comment:28

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.

@rwst

This comment has been minimized.

@kcrisman
Copy link
Member

comment:30

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.

@rwst
Copy link

rwst commented Jun 28, 2014

@rwst
Copy link

rwst commented Jun 28, 2014

Author: Ralf Stephan

@rwst
Copy link

rwst commented Jun 28, 2014

Commit: 518de3e

@rwst
Copy link

rwst commented Jun 28, 2014

comment:32

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

@rwst
Copy link

rwst commented Jun 28, 2014

Changed dependencies from #8734, #16007 to none

@rwst

This comment has been minimized.

@kcrisman
Copy link
Member

comment:34

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.

@rwst
Copy link

rwst commented Jun 30, 2014

comment:35

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.

@rwst
Copy link

rwst commented Jun 30, 2014

comment:36

Replying to @rwst:

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.

@kcrisman
Copy link
Member

comment:37
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.

@kcrisman
Copy link
Member

kcrisman commented Jul 2, 2014

Reviewer: Karl-Dieter Crisman

@kcrisman
Copy link
Member

kcrisman commented Jul 2, 2014

comment:38

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.

@vbraun
Copy link
Member

vbraun commented Jul 3, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants