Ticket #8479 (closed enhancement: fixed)
numpy support for more basic functions
| Reported by: | whuss | Owned by: | AlexGhitza |
|---|---|---|---|
| Priority: | major | Milestone: | sage-4.4.2 |
| Component: | symbolics | Keywords: | numpy |
| Cc: | jason | Work issues: | |
| Report Upstream: | N/A | Reviewers: | Burcin Erocal |
| Authors: | Wilfried Huss | Merged in: | sage-4.4.2.alpha0 |
| Dependencies: | Stopgaps: |
Description (last modified by whuss) (diff)
The attached patch adds numpy support for the functions:
coth, sech, csch, arccoth, arcsech, arccsch, ceil, floor, sqrt, real_part, imag_part, sec, csc, cot, arccot, arccsc, arcsec, atan2
Attachments
-
trac-8479-numpy_functions.patch
(12.7 KB) -
added by whuss 3 years ago.
-
trac-8479-numpy_functions.take2.patch
(12.4 KB) -
added by burcin 3 years ago.
- apply only this patch
Change History
comment:2 Changed 3 years ago by burcin
- Status changed from new to needs_review
- Summary changed from [with patch, needs review] numpy support for more basic functions to numpy support for more basic functions
- Component changed from basic arithmetic to symbolics
- Authors changed from whuss to Wilfried Huss
I hadn't seen this before! It's an enhancement to symbolic functions, so I'm changing the component to symbolics.
We don't use tags on the subject line to mark tickets for review anymore, see the Action options right above the Submit button.
comment:4 follow-up: ↓ 5 Changed 3 years ago by burcin
- Status changed from needs_review to needs_info
- Reviewers set to Burcin Erocal
The patch looks really good and addresses an important problem. I have a few minor remarks/questions before I give a positive review:
- Can we change the test in sage.functions.other.sqrt() to work without importing numpy? I didn't check the effects on performance, but sqrt() gets used a lot, so keeping it free of numpy unless absolutely necessary would be good.
- All the examples in the doctests are for functions with a single argument. Is there any reason to move the check in sage.symbolic.function.Function.__call__() to try all arguments? We should also consider moving this check to sage.symbolic.function.BuiltinFunction.__call__().
comment:5 in reply to: ↑ 4 ; follow-up: ↓ 6 Changed 3 years ago by whuss
- Status changed from needs_info to needs_review
- Description modified (diff)
Replying to burcin:
- Can we change the test in sage.functions.other.sqrt() to work without importing numpy?
done in the new patch
- All the examples in the doctests are for functions with a single argument. Is there any reason to move the check in sage.symbolic.function.Function.__call__() to try all arguments?
In the original patch I forgot to add tests for atan2, which also didn't work with numpy before. The tests are now included.
We should also consider moving this check to sage.symbolic.function.BuiltinFunction.__call__().
Why would this be better? It currently does not have a __call__ method.
Changed 3 years ago by burcin
-
attachment
trac-8479-numpy_functions.take2.patch
added
apply only this patch
comment:6 in reply to: ↑ 5 Changed 3 years ago by burcin
- Status changed from needs_review to positive_review
Thanks for the quick response.
Replying to whuss:
Replying to burcin:
- Can we change the test in sage.functions.other.sqrt() to work without importing numpy?
done in the new patch
Thanks!
- All the examples in the doctests are for functions with a single argument. Is there any reason to move the check in sage.symbolic.function.Function.__call__() to try all arguments?
In the original patch I forgot to add tests for atan2, which also didn't work with numpy before. The tests are now included.
Fair enough.
We should also consider moving this check to sage.symbolic.function.BuiltinFunction.__call__().
Why would this be better?
This code path is speed critical since it gets called thousands of times for many applications like plotting. Checking the type of each argument introduces a penalty for functions with a single argument.
It currently does not have a __call__ method.
Sorry about that. I confused it with the __call__() method of GinacFunction, which also checks if there is only one argument, and it has a method with the same name as the function. Now I wonder why isn't that in BuiltinFunction...
Your patch was missing an empty line after line 165 in sage/functions/other.py. attachment:trac-8479-numpy_functions.take2.patch fixes this. Only attachment:trac-8479-numpy_functions.take2.patch should be applied.
Great work!
