Mostly fix slow down by #30022

Question

Mostly fix slow down by #30022

Closed this issue 4 years ago · 15 comments

We avoid multiple imports caused by #30022:

Before (and with #30022):

sage: a = 1/1000
sage: %timeit a.__pari__()
810 ns ± 2.33 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
sage: p = a.__pari__()
sage: %timeit QQ(p)
1.52 µs ± 1.37 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
sage: a = 12314
sage: %timeit a.__pari__()
723 ns ± 0.804 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
sage: p = a.__pari__()
sage: %timeit ZZ(p)
715 ns ± 1.32 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
sage: %timeit a.divisors(method='pari')
1.23 µs ± 1.51 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
sage: %timeit a.is_prime_power()
647 ns ± 0.164 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
sage: %timeit a.is_prime()
604 ns ± 0.486 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
sage: %timeit a.factor()
3.6 µs ± 13.4 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

Reverting #30022:

sage: a = 1/1000
sage: %timeit a.__pari__()
221 ns ± 0.289 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
sage: p = a.__pari__()
sage: %timeit QQ(p)
810 ns ± 0.852 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
sage: a = 12314
sage: %timeit a.__pari__()
176 ns ± 0.217 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
sage: p = a.__pari__()
sage: %timeit ZZ(p)
150 ns ± 0.104 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
sage: %timeit a.divisors(method='pari')
643 ns ± 1.38 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
sage: %timeit a.is_prime_power()
53.9 ns ± 0.336 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
sage: %timeit a.is_prime()
58 ns ± 0.174 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
sage: %timeit a.factor()
2.99 µs ± 4.62 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

With this ticket:

sage: a = 1/1000
sage: %timeit a.__pari__()
274 ns ± 0.344 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
sage: p = a.__pari__()
sage: %timeit QQ(p)
867 ns ± 0.999 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
sage: a = 12314
sage: %timeit a.__pari__()
200 ns ± 0.0656 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
sage: p = a.__pari__()
sage: %timeit ZZ(p)
183 ns ± 0.179 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
sage: %timeit a.divisors(method='pari')
673 ns ± 0.928 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
sage: %timeit a.is_prime_power()
88.4 ns ± 0.075 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
sage: %timeit a.is_prime()
68.9 ns ± 0.0878 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
sage: %timeit a.factor()
3.19 µs ± 10.2 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

CC: @tscrim

Component: refactoring

Author: Jonathan Kliem

Branch/Commit: 4d7ac2f

Reviewer: Matthias Koeppe, Samuel Lelièvre

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

Answer 1 · 2021-10-26T13:03:14.000Z

Branch: u/gh-kliem/fix_regression_of_30022

Answer 2 · 2021-10-26T13:03:14.000Z

Commit: 1589f94

Answer 3 · 2021-10-26T13:03:14.000Z

New commits:

`33e6c65`	`improve conversion rational <-> pari`
`1589f94`	`improve conversion integer <-> pari`

Answer 4 · 2021-10-26T17:52:36.000Z

comment:2

The test failures are not from this ticket:

sage -t --long --random-seed=29194783039255016302038928490654063942 src/sage/rings/integer.pyx  # 1 doctest failed
sage -t --long --random-seed=29194783039255016302038928490654063942 src/sage/modular/modform/numerical.py  # 3 doctests failed

Answer 5 · 2021-10-26T17:53:00.000Z

Reviewer: Matthias Koeppe

Answer 6 · 2021-10-26T17:53:00.000Z

comment:3

Thanks for working on this! LGTM.

Answer 7 · 2021-10-26T19:17:49.000Z

comment:4

Thank you.

Answer 8 · 2021-10-27T12:46:57.000Z

comment:5

Not sure about set_integer_from_pari_gen vs set_integer_from_gen.

Answer 9 · 2021-10-27T13:41:30.000Z

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

`4d7ac2f`	`fixup`

Answer 10 · 2021-10-27T13:41:30.000Z

Changed commit from 1589f94 to 4d7ac2f

Answer 11 · 2021-10-27T13:42:02.000Z

Description changed:

--- 
+++ 
@@ -64,7 +64,7 @@
 200 ns ± 0.0656 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
 sage: p = a.__pari__()
 sage: %timeit ZZ(p)
-716 ns ± 0.912 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
+183 ns ± 0.179 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
 sage: %timeit a.divisors(method='pari')
 673 ns ± 0.928 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
 sage: %timeit a.is_prime_power()

Answer 12 · 2021-10-27T18:07:09.000Z

comment:8

That seems better. Another option would have been to use

from sage.libs.pari.convert_sage import (
    set_integer_from_gen as set_integer_from_pari_gen)

if that name mattered.

Answer 13 · 2021-10-27T18:07:09.000Z

Changed reviewer from Matthias Koeppe to Matthias Koeppe, Samuel Lelièvre

Answer 14 · 2021-10-27T18:22:54.000Z

comment:9

It was just a typo and made this whole construction pointless there. This is the reason, I modified the benchmarks as well.

Answer 15 · 2021-10-31T22:20:34.000Z

Changed branch from u/gh-kliem/fix_regression_of_30022 to 4d7ac2f