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

Mostly fix slow down by #30022 #32775

Closed
kliem opened this issue Oct 26, 2021 · 15 comments
Closed

Mostly fix slow down by #30022 #32775

kliem opened this issue Oct 26, 2021 · 15 comments

Comments

@kliem
Copy link
Contributor

kliem commented Oct 26, 2021

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

@kliem kliem added this to the sage-9.5 milestone Oct 26, 2021
@kliem
Copy link
Contributor Author

kliem commented Oct 26, 2021

Branch: u/gh-kliem/fix_regression_of_30022

@kliem
Copy link
Contributor Author

kliem commented Oct 26, 2021

Commit: 1589f94

@kliem
Copy link
Contributor Author

kliem commented Oct 26, 2021

New commits:

33e6c65improve conversion rational <-> pari
1589f94improve conversion integer <-> pari

@mkoeppe
Copy link
Member

mkoeppe commented Oct 26, 2021

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

@mkoeppe
Copy link
Member

mkoeppe commented Oct 26, 2021

Reviewer: Matthias Koeppe

@mkoeppe
Copy link
Member

mkoeppe commented Oct 26, 2021

comment:3

Thanks for working on this! LGTM.

@kliem
Copy link
Contributor Author

kliem commented Oct 26, 2021

comment:4

Thank you.

@slel
Copy link
Member

slel commented Oct 27, 2021

comment:5

Not sure about set_integer_from_pari_gen vs set_integer_from_gen.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 27, 2021

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

4d7ac2ffixup

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 27, 2021

Changed commit from 1589f94 to 4d7ac2f

@kliem

This comment has been minimized.

@slel
Copy link
Member

slel commented Oct 27, 2021

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.

@slel
Copy link
Member

slel commented Oct 27, 2021

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

@kliem
Copy link
Contributor Author

kliem commented Oct 27, 2021

comment:9

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

@vbraun
Copy link
Member

vbraun commented Oct 31, 2021

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

mkoeppe added a commit to mkoeppe/sage that referenced this issue Feb 12, 2023
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

4 participants