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

maxima interface gives precedence to function dictionary instead of local variables #7661

Closed
burcin opened this issue Dec 11, 2009 · 17 comments

Comments

@burcin
Copy link

burcin commented Dec 11, 2009

From the sage-devel thread:

http://groups.google.com/group/sage-devel/t/c89582242c83a349

On Fri, 11 Dec 2009 13:46:31 +0100
Nathann Cohen <nathann.cohen@gmail.com> wrote:

> sage: var('delta k')
> sage: m1=2*delta**2 + 2**2*delta*k
> sage: n=delta*k+2
> sage: m2=(2*delta)**2+(k-1)*4
> sage: m=(delta+delta*k-(delta-1))
> sage: ((m1/n)-(m2/n)).expand().simplify()

On 4.3.rc0, I get this:

TypeError: unsupported operand parent(s) for '*': 'Symbolic Ring' and
'<class 'sage.functions.generalized.FunctionDiracDelta'>'

The Maxima interface seems to give precedence to the global function
dictionary instead of the local variables when converting Maxima output
back to Sage expressions.

sage: dirac_delta(x)
dirac_delta(x)
sage: maxima(dirac_delta(x))
delta(x)

CC: @robert-marik

Component: interfaces

Keywords: maxima

Author: Burcin Erocal

Reviewer: Robert Mařík

Merged: sage-4.4.alpha0

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

@williamstein
Copy link
Contributor

comment:1

People run into this all the time, evidently:

[15:21] --> SageWWW has joined this channel (~SageWWW@64.241.37.140).
[15:23] <SageWWW> hey guys.  what do you think about http://pastebin.ca/1772520
[15:24] <SageWWW> d = var('delta'), so now d is a reference to a sage.symbolic.expression.Expression
[15:25] <SageWWW> but when we try to add it to something else, it thinks its a sage.functions.generalized.FunctionDiracDelta
[15:27] <wstein> https://github.com/sagemath/sage-prod/issues/7661

@sagetrac-eigenlambda
Copy link
Mannequin

sagetrac-eigenlambda mannequin commented Jan 31, 2010

comment:2

sage: d = var('delta')
sage: e = d.maxima()
sage: sage.calculus.calculus.symbolic_expression_from_maxima_element(e)
dirac_delta

somewhere in symbolic_expression_from_maxima_element(), the string from maxima is looked up in sage.calculus.calculus._syms, which by default has 'delta': dirac_delta . So this is what's happening, next, SR() barfs on trying to turn dirac_delta into a symbolic expression, at which point people who just wanted their variable 'delta' back get confused and frustrated.

sage: del sage.calculus.calculus._syms['delta']
sage: sage.calculus.calculus.symbolic_expression_from_maxima_element(e)
delta

That may not be such a good idea, however, since what sage calls dirac_delta, maxima refers to as delta. Nevertheless, since reset('delta') appears to remove delta from that dictionary, perhaps var('delta') should also do so?

Of course, what happens when someone does a Laplace transform with delta as a sage variable will then come out confusing and wrong. At least the current behavior is merely broken.

@burcin
Copy link
Author

burcin commented Apr 5, 2010

comment:3

attachment: trac_7661-maxima_convert_back.patch fixes the problem reported above, and a similar problem with function conversions back from maxima reported in #8459 comment:2.

@burcin
Copy link
Author

burcin commented Apr 5, 2010

Author: Burcin Erocal

@burcin
Copy link
Author

burcin commented Apr 6, 2010

comment:4

Attachment: trac_7661-maxima_convert_back.patch.gz

I updated attachment: trac_7661-maxima_convert_back.patch to remove a doctest fix broken by a previous patch in my queue (#6949, symbol... line in sage/symbolic/ring.pyx).

This patch depends on #7748.

@robert-marik
Copy link
Mannequin

robert-marik mannequin commented Apr 9, 2010

comment:5

Thanks for working onthis. Is #7748 the only prerequisity? I installed three patches as described at #7748 and got the following error

patching file sage/calculus/calculus.py
Hunk #3 succeeded at 1414 with fuzz 1 (offset -4 lines).
Hunk #5 FAILED at 1455
1 out of 14 hunks FAILED -- saving rejects to file sage/calculus/calculus.py.rej
abort: patch failed to apply

@burcin
Copy link
Author

burcin commented Apr 9, 2010

comment:6

AFAICT, #8237 also changes that code. Can you try with #8237 applied?

I'm sorry for the dependency hell we get into with these patches for every release. I don't know any way to automatically get a list of dependencies for a patch in my queue.

Thanks for your time Robert.

@burcin burcin assigned burcin and unassigned williamstein Apr 9, 2010
@robert-marik
Copy link
Mannequin

robert-marik mannequin commented Apr 9, 2010

comment:7

Hello Burcin

I think that two lines should be removed from the patch

global _syms 
_syms = sage.symbolic.pynac.symbol_table.get('maxima', {}) 

I updated your patch, it is now http://user.mendelu.cz/marik/sage/trac_7661-maxima_convert_back2.patch

If everything will work, I'll return in few (several) hours with positive review (tests in functions, interfaces, symbolics and calculus passed, now running all the test).

Robert

@burcin
Copy link
Author

burcin commented Apr 9, 2010

comment:8

OK. That is one approach to solving this problem. Now we need to rebase the patch at #8237 so that it applies on top of these. Removing the offending hunk from calculus.py should be enough for that.

Note that your updated patch shows you as the author. I'd appreciate it if you changed that back.

Thanks.

@robert-marik
Copy link
Mannequin

robert-marik mannequin commented Apr 9, 2010

comment:9

Sure, it was intended as temporary patch and from this reason I did not upload to trac server unless tested. I got some doctest failures in three files. See http://boxen.math.washington.edu/home/marik/ and the files a, b and c.

I think that b is simple to fix, but have no idea about a and c.

@burcin
Copy link
Author

burcin commented Apr 9, 2010

Attachment: trac_7661-maxima_convert_back.take2.patch.gz

apply only this patch

@burcin
Copy link
Author

burcin commented Apr 9, 2010

comment:11

Thanks a lot for the quick feedback.

@robert-marik
Copy link
Mannequin

robert-marik mannequin commented Apr 10, 2010

comment:12

I tested it on a fresh install and seems that all a,b,c are resolved.
I am running all tests again, to be sure :)

@robert-marik
Copy link
Mannequin

robert-marik mannequin commented Apr 10, 2010

comment:13

Tests passed, postive review, thanks for fixing - very very usefull ticket.

Release manager: apply only trac_7661-maxima_convert_back.take2.patch

@robert-marik
Copy link
Mannequin

robert-marik mannequin commented Apr 10, 2010

Reviewer: Robert Mařík

@robert-marik robert-marik mannequin removed the s: needs review label Apr 10, 2010
@jhpalmieri
Copy link
Member

comment:14

Merged "trac_7661-maxima_convert_back.take2.patch" in 4.4.alpha0

@jhpalmieri
Copy link
Member

Merged: sage-4.4.alpha0

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

3 participants