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

Speed up coercions #27379

Closed
mezzarobba opened this issue Feb 27, 2019 · 5 comments
Closed

Speed up coercions #27379

mezzarobba opened this issue Feb 27, 2019 · 5 comments

Comments

@mezzarobba
Copy link
Member

  • Don't call _repr_() on the parents every time a coercion or conversion fails. (One could consider making Parent.__repr__() keep the name in cache instead, but we probably don't want that for things like finite fields, and in any case __repr__() is handled by SageObject at the moment.)
  • Add a fast path for trivial coercions in Parent.coerce(foo).

Before:

sage: R.<x,y,z> = ZZ[]
sage: a = GF(3)(2)
sage: %timeit x(a,a,a)
The slowest run took 37.44 times longer than the fastest. This could mean that an intermediate result is being cached.
100000 loops, best of 3: 13.1 µs per loop

After:

sage: R.<x,y,z> = ZZ[]
sage: a = GF(3)(2)
sage: %timeit x(a,a,a)
The slowest run took 162.14 times longer than the fastest. This could mean that an intermediate result is being cached.
100000 loops, best of 3: 3.98 µs per loop

Component: performance

Author: Marc Mezzarobba

Branch/Commit: 3b49d0a

Reviewer: Travis Scrimshaw

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

@mezzarobba mezzarobba added this to the sage-8.7 milestone Feb 27, 2019
@tscrim

This comment has been minimized.

@tscrim
Copy link
Collaborator

tscrim commented Feb 27, 2019

comment:2

LGTM.

I do not think we want to cache the result of Parent.__repr__ because that is designed so you can dynamically change the repr of the object via foo.rename('bar').

@tscrim
Copy link
Collaborator

tscrim commented Feb 27, 2019

Reviewer: Travis Scrimshaw

@mezzarobba
Copy link
Member Author

comment:3

Replying to @tscrim:

LGTM.

Thank you!

I do not think we want to cache the result of Parent.__repr__ because that is designed so you can dynamically change the repr of the object via foo.rename('bar').

Yes, that's an additional reason, though one could imagine making rename() update the cache.

@vbraun
Copy link
Member

vbraun commented Mar 2, 2019

Changed branch from u/mmezzarobba/optimize_coerce to 3b49d0a

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

3 participants