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

faster coercion from Integer to Quadratic Number fields #14563

Closed
videlec opened this issue May 10, 2013 · 6 comments
Closed

faster coercion from Integer to Quadratic Number fields #14563

videlec opened this issue May 10, 2013 · 6 comments

Comments

@videlec
Copy link
Contributor

videlec commented May 10, 2013

As suggested in this thread on sage-devel, the patch implements a faster coercion from the integers to quadratic number fields.

To avoid rebase I put dependencies on #13213 and #13256.

Depends on #13213
Depends on #13256

Component: coercion

Keywords: quadratic number field

Author: ​Vincent Delecroix

Reviewer: Volker Braun

Merged: sage-5.12.beta1

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

@videlec videlec added this to the sage-5.11 milestone May 10, 2013
@videlec videlec self-assigned this May 10, 2013
@vbraun
Copy link
Member

vbraun commented Jul 8, 2013

comment:3

There is a block of commented-out code under

# we ignore the code below because of the morphism 

If its not used then delete it, don't leave commented-out chunks of code lying around. Is there a performance difference with this branch removed? It looks like an attempt to deliberately circumvent the morphism to me.

@videlec
Copy link
Contributor Author

videlec commented Jul 8, 2013

comment:4

Thanks for having a look.

I commented the code because it was never used because of the coercion system (see the __call__ of Parent). It was the case even before my patch. I think the gain is minor but the fact is that it was unused. I will remove it.

Here are some timings, the improvement is somwhere around x10.

without

sage: K=QuadraticField(2)
sage: q = 12   
sage: %timeit K(q)
100000 loops, best of 3: 10.3 us per loop
sage: g = K.gen()
sage: %timeit q*g
1000000 loops, best of 3: 1.66 us per loop
sage: %timeit q+g
100000 loops, best of 3: 13.2 us per loop

with

sage: K=QuadraticField(2)
sage: q = 12   
sage: %timeit K(q)
1000000 loops, best of 3: 660 ns per loop
sage: g = K.gen()
sage: %timeit q*g
1000000 loops, best of 3: 1.74 us per loop
sage: %timeit q+g
100000 loops, best of 3: 2.76 us per loop

There is no difference for the multiplication because the methods _lmul_ or _rmul_ are called directly and there is not a call to the coercion morphism.

@vbraun
Copy link
Member

vbraun commented Jul 9, 2013

comment:5

Attachment: trac_14563-Z_to_quadratic_field.patch.gz

Looks good to me...

@vbraun
Copy link
Member

vbraun commented Jul 9, 2013

Reviewer: Volker Braun

@jdemeyer
Copy link

jdemeyer commented Aug 2, 2013

Changed author from vdelecroix to ​Vincent Delecroix

@jdemeyer
Copy link

Merged: sage-5.12.beta1

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