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

Make evaluation possible for 'hold' objects #10034

Closed
kcrisman opened this issue Sep 29, 2010 · 27 comments
Closed

Make evaluation possible for 'hold' objects #10034

kcrisman opened this issue Sep 29, 2010 · 27 comments

Comments

@kcrisman
Copy link
Member

See #9879, where it is now possible to 'hold' a symbolic expression:

sage: a = (pi/12).tan(hold=True)
sage: a
tan(1/12*pi)

However, without going through Maxima and a.simplify(), it isn't clear how to get the actual answer for this. Either by changing simplify() to try simplifying through Pynac first, or by adding something like an a.eval() method, we should make that possible without Maxima.

CC: @eviatarbach @paulmasson

Component: symbolics

Author: Eviatar Bach, Ralf Stephan

Branch/Commit: 6e4c716

Reviewer: Paul Masson

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

@kcrisman
Copy link
Member Author

kcrisman commented Oct 6, 2010

comment:1

This is related to #10071, which is about functions which can't even be evaluated using Maxima or n(). This ticket is saying that one should be able to evaluate all held functions without using Maxima or n(), whether or not a function currently can be evaluated in that way or not.

@eviatarbach
Copy link

comment:3

Here's a patch which makes evaluation possible, simply by walking the expression and evaluating all operations. It does not work for the functions in #10071, because the .operator() method doesn't work on them; I believe this is a separate issue for another ticket though.

Patchbot apply trac_10034.patch

@eviatarbach
Copy link

Attachment: trac_10034.patch.gz

@kcrisman
Copy link
Member Author

kcrisman commented Jul 3, 2013

comment:4

Glance looks good. Before I think more about this, a question - is eval the right name for this? I know I'm the one who suggested it... but what do other eval methods do? Also, I think there are a lot of examples which use simplify to evaluate these currently - maybe we could switch them to this (or add this, perhaps). Yes, I agree that #10071 is fine not to try to handle here - that's why I opened #10071.

@eviatarbach
Copy link

comment:5

The _eval_ method for symbolic functions defines what to do when the function is evaluated, and the _eval_self method for expressions tries to do numerical evaluation. Maybe a name like unhold would be better?

Ah right, I'll switch the examples to this. There's no reason why these expressions should be transferred to Maxima simply for evaluating an operation.

@kcrisman
Copy link
Member Author

kcrisman commented Jul 3, 2013

comment:6

Ok.

As another remark (and you might hate me for this one), I could imagine someone wanting to evaluate some but not all of the held operations. I think this is possible with your patch and judicious use of op and the tree, but at least adding an example of that would be helpful. Particularly in the x * x + x * x example, though, I wonder if it wouldn't be pretty annoying to simplify this to 2 * x * x using this. What do you think about such cases?

@eviatarbach
Copy link

comment:7

I've thought about this. One option is to use pattern matching to specify the parts to evaluate, but I don't how the expression could be reconstructed afterwards.

Another option is to have an argument for providing a list of operators not to evaluate (I think it's better to have an argument to exclude rather than include, because it is difficult to find all the operators in Sage, while finding ones to exclude just involves searching the expression). Then for the 2 * x * x example you could just add operator.mul to the excluded operators and it would work.

@eviatarbach
Copy link

comment:8

Okay, the new patch renames eval to unhold, moves the examples to use the new method, and adds an exclude option. Excluding arithmetic operators doesn't yet work because of #14850.

Patchbot apply trac_10034_2.patch

@eviatarbach
Copy link

Attachment: trac_10034_2.patch.gz

@eviatarbach
Copy link

comment:9

Actually, the way I implemented it is not correct, since if the length of the operands is over two it reduces the rest of the operands using the operator. This is the desired behaviour for the arithmetic operators, but not generally.

@kcrisman
Copy link
Member Author

comment:11

Maybe this should also fix other places the hold stuff is shown, e.g. functions/trig.py.

@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014
@rwst
Copy link

rwst commented Dec 25, 2014

comment:15

I don't think an eval member is right here. The end user would expect .simplify_trig() to work, and it does actually. The only problem the original submitter had was the Maxima overhead, so it boils down to a native implementation called from simplify_trig().

@rwst rwst changed the title Make evaluation possible for Pynac 'hold' objects simplify_trig of f(a/b*pi) without Maxima Dec 25, 2014
@rwst
Copy link

rwst commented Jul 27, 2015

comment:16

What I wrote:

I don't think an eval member is right here. The end user would expect .simplify_trig() to work, and it does actually. The only problem the original submitter had was the Maxima overhead, so it boils down to a native implementation called from simplify_trig().

is of course nonsense. Every function that sends the held expression through Maxima unchanged would work because the hold status gets lost. The expansion happens in Pynac's (function)::eval so is already implemented outside Maxima.

@rwst
Copy link

rwst commented Oct 22, 2015

comment:17

Previous title restored.

@rwst rwst changed the title simplify_trig of f(a/b*pi) without Maxima Make evaluation possible for Pynac 'hold' objects Oct 22, 2015
@rwst
Copy link

rwst commented Oct 23, 2015

@rwst
Copy link

rwst commented Oct 23, 2015

Commit: 026ab3d

@rwst
Copy link

rwst commented Oct 23, 2015

Author: Eviatar Bach, Ralf Stephan

@rwst
Copy link

rwst commented Oct 23, 2015

comment:19

I used the convenient ExpressionTreeWalker that takes care of the caveats mentioned above.


New commits:

026ab3d10034: implement Expression.unhold()

@rwst rwst modified the milestones: sage-6.4, sage-6.10 Oct 23, 2015
@rwst rwst changed the title Make evaluation possible for Pynac 'hold' objects Make evaluation possible for 'hold' objects Oct 23, 2015
@rwst
Copy link

rwst commented Jun 13, 2016

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 13, 2016

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

d414b5d10034: increase coverage

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 13, 2016

Changed commit from 026ab3d to d414b5d

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Jun 24, 2016

comment:22

Everything I tested functions just fine, including building documents. Ready for positive review.

One minor quibble: the exclude option references Trac #14850, which is a closed duplicate of #10169. Shouldn't that be updated?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 25, 2016

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

7b90350Merge branch 'develop' into t/10034/10034-1
6e4c716minor cosmetics

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 25, 2016

Changed commit from d414b5d to 6e4c716

@rwst
Copy link

rwst commented Jun 25, 2016

comment:25

Right. Thanks for the review. Please add your name to Reviewers: too.

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Jun 25, 2016

Reviewer: Paul Masson

@vbraun
Copy link
Member

vbraun commented Jun 26, 2016

Changed branch from u/rws/10034-1 to 6e4c716

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

6 participants