#15071 closed enhancement (fixed)
Make it easy to have automatic numerical evaluation of symbolic functions on inexact input
Reported by: | eviatarbach | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-5.13 |
Component: | symbolics | Keywords: | |
Cc: | burcin, benjaminfjones | Merged in: | sage-5.13.beta0 |
Authors: | Eviatar Bach | Reviewers: | Benjamin Jones |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
This ticket is for making it easy to have symbolic functions automatically evaluate on inexact input. This is already the behaviour of most symbolic functions and as such it would be good to have a standard way of implementing it.
This ticket modifies Function._eval_default
to accept an arbitrary number of arguments. To have the numeric evaluation work, _eval_default
can be assigned to _eval_
, or it can be called from within _eval_
and, if it returns None
, leaves opportunity for exact simplification. For example,
sage: from sage.symbolic.function import BuiltinFunction sage: class Test(BuiltinFunction): ....: def __init__(self): ....: BuiltinFunction.__init__(self, 'test', nargs=1) ....: def _evalf_(self, x, parent): ....: return 0.5 ....: def _eval_(self, x): ....: y = self._eval_default(x) ....: if y: ....: return y ....: else: ....: return 3 sage: test = Test() sage: test(1.3) 0.500000000000000 sage: test(pi) 3
Apply trac15071_4.patch
Attachments (4)
Change History (22)
Changed 7 years ago by
comment:1 Changed 7 years ago by
comment:2 Changed 7 years ago by
- Status changed from new to needs_review
comment:3 Changed 7 years ago by
Good idea!
My initial impression is that your patch looks good, but I don't have time right now to do a thorough review.
The only immediate change I would suggest is to separate the changes to hyperbolic.py
and expression.py
into a different ticket and patch. It's annoying, I know, but it's better to separate issues in the long run.
comment:4 Changed 7 years ago by
Okay thank you!
I can separate one of the changes to hyperbolic.py
, but the other one is necessary to keep this patch from failing tests, and indeed to not break many comparisons involving hyperbolic functions. Can I keep that one in this patch?
Changed 7 years ago by
comment:5 Changed 7 years ago by
Actually I meant all the changes to hyperbolic.py
could be reverted (now #15134), but not the one to expression.pyx
. New patch removes everything not necessary for passing doctests.
Note that I changed the cot
tests to coth
since cot
wasn't actually testing the function, because it doesn't use _eval_default
.
Patchbot apply trac15071_2.patch
comment:6 Changed 7 years ago by
Ok, makes sense. Thanks for doing the separation.
I looked in detail at your latest patch and it looks good. One thing I'd like to see before it's merged is more test coverage. It looks like only the len(args) == 1
branch is tested right now. We should include tests of the multiple argument case and also the case of multiple arguments with distinct parents. Of course, once we have symbolic functions that use this _eval_default
the toy test classes like you've included won't be necessary.
After that, I'd say this is good to go.
comment:7 Changed 7 years ago by
Thank you! Here's a version with more tests.
Changed 7 years ago by
comment:8 Changed 7 years ago by
Apparently excepting the AttributeError
fixes other issues; see http://ask.sagemath.org/question/2957/strange-attributeerror. I would add a test for that but I guess it's not within the scope of the patch; maybe I'll just create a new ticket and mark it as fixed by this one.
comment:9 Changed 7 years ago by
It looks like now, only the second branch (the else) is being tested. Can we add the original test you had as well as the new one?
About the AttributeError
issue, what you propose sounds reasonable to me, but it excludes the possibility of getting a test in the codebase (since the other ticket wouldn't get to include a patch). Since excepting the error is done here I think it's probably okay to include a test as an "aside". What do you think?
comment:10 Changed 7 years ago by
Sounds good! New patch makes the suggested changes.
Changed 7 years ago by
comment:11 Changed 7 years ago by
Looks great. After tests come back positive from the bot, I'll give this a positive review.
patchbot, apply trac15071_4.patch
comment:12 Changed 7 years ago by
Old patchbot seems to be on vacation. I'll do the testing myself..
comment:13 Changed 7 years ago by
- Description modified (diff)
- Status changed from needs_review to positive_review
All tests pass! Positive review.
comment:14 Changed 7 years ago by
Thank you so much for reviewing! Now functions that are doing this manually can be transitioned.
comment:15 Changed 7 years ago by
- Milestone changed from sage-5.12 to sage-5.13
Please add your real names as Author and Reviewer.
comment:16 Changed 7 years ago by
- Reviewers set to Benjamin F. Jones
comment:17 Changed 7 years ago by
- Merged in set to sage-5.13.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
comment:18 Changed 7 years ago by
- Reviewers changed from Benjamin F. Jones to Benjamin Jones
Benjamin, I CC'ed you since it appears that you implemented this for many symbolic functions. What do you think of this approach?
The reason why I added
AttributeError
to theexcept
intest_relation
is because this is necessary to make comparisons work with some of the hyperbolic functions. Before it worked because they wouldn't evaluate automatically withCIF
input, but they should since it's inexact.I also added a small fix to make the hyperbolic functions work with the Python
complex
type; they didn't before.Patchbot apply trac15071.patch