-
-
Notifications
You must be signed in to change notification settings - Fork 481
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
speed regression with fast_callable #5616
Comments
Attachment: trac5616-fast-callable-gen-pxd.patch.gz |
Attachment: trac5616-fast-callable-cdef-interface.patch.gz |
comment:1
The attached patches should fix this problem. (Apply both patches, in order.) I split the fix into two logically separate pieces. The first only generates .pxd files for the fast_callable interpreters; this should have no observable effect. The second adds a .call_c() cdef method to the interpreters. (Each interpreter has its own call_c, rather than inheriting a common method as Robert suggested; I did it this way because I like typechecking.) The second patch also modifies parametric_surface.pyx to take advantage of the call_c() method when generating the surface. I left the old code in as well; this means you can benchmark fast_eval vs. fast_callable by setting sage.ext.fast_eval.new_fast_float to False/True. These patches depend on #5622. |
comment:2
Excellent. Yes, generating specific pxd files with typed call_c methods is a better way to go. Code is good and this completely resolves the issue. |
comment:4
Merged both patches in Sage 3.4.1.rc0. Cheers, Michael |
Before (vanilla 3.4)
after (3.4 + #5093)
I think this is due to there not being an interface to evaluate fast_callable objects without passing through Python. Perhaps a
method should be attached to the generic interpreter wrapper class (to be overridden by the subclasses), and those with specific knowledge about the various implementations could then use this interface (e.g. RDF passes double*).
Component: basic arithmetic
Issue created by migration from https://trac.sagemath.org/ticket/5616
The text was updated successfully, but these errors were encountered: