Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#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 benjaminfjones)

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)

trac15071.patch (5.2 KB) - added by eviatarbach 7 years ago.
trac15071_2.patch (3.7 KB) - added by eviatarbach 7 years ago.
trac15071_3.patch (4.0 KB) - added by eviatarbach 7 years ago.
trac15071_4.patch (4.9 KB) - added by eviatarbach 7 years ago.

Download all attachments as: .zip

Change History (22)

Changed 7 years ago by eviatarbach

comment:1 Changed 7 years ago by eviatarbach

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 the except in test_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 with CIF 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

Last edited 7 years ago by eviatarbach (previous) (diff)

comment:2 Changed 7 years ago by eviatarbach

  • Status changed from new to needs_review

comment:3 Changed 7 years ago by benjaminfjones

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 eviatarbach

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 eviatarbach

comment:5 Changed 7 years ago by eviatarbach

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 benjaminfjones

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 eviatarbach

Thank you! Here's a version with more tests.

Changed 7 years ago by eviatarbach

comment:8 Changed 7 years ago by eviatarbach

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 benjaminfjones

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 eviatarbach

Sounds good! New patch makes the suggested changes.

Changed 7 years ago by eviatarbach

comment:11 Changed 7 years ago by benjaminfjones

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 benjaminfjones

Old patchbot seems to be on vacation. I'll do the testing myself..

comment:13 Changed 7 years ago by benjaminfjones

  • Description modified (diff)
  • Status changed from needs_review to positive_review

All tests pass! Positive review.

comment:14 Changed 7 years ago by eviatarbach

Thank you so much for reviewing! Now functions that are doing this manually can be transitioned.

comment:15 Changed 7 years ago by jdemeyer

  • 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 eviatarbach

  • Authors set to Eviatar Bach
  • Reviewers set to Benjamin F. Jones

comment:17 Changed 7 years ago by jdemeyer

  • 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 jdemeyer

  • Reviewers changed from Benjamin F. Jones to Benjamin Jones
Note: See TracTickets for help on using tickets.