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

Fast function field arithmetic #9051

Closed
robertwb opened this issue May 26, 2010 · 29 comments
Closed

Fast function field arithmetic #9051

robertwb opened this issue May 26, 2010 · 29 comments

Comments

@robertwb
Copy link
Contributor

Followup to #7585, which also did many, many other things.

Wrapping flint directly is much faster than the current implementation of Frac(GF(p)['t'])

CC: @mminzlaff

Component: algebra

Author: Robert Bradshaw, David Roe, William Stein

Reviewer: Robert Bradshaw, William Stein

Merged: sage-4.5.2.alpha0

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

@robertwb
Copy link
Contributor Author

Attachment: 9051-FpT_1.patch.gz

@robertwb
Copy link
Contributor Author

Attachment: 9051-FpT_2.patch.gz

Attachment: 9051-FpT_3.patch.gz

@robertwb
Copy link
Contributor Author

comment:1

Apply all three patches in order.

Positive review to 9051-FpT_2.patch (the third was somewhat a rebasing, referee, and fixing some stuff).

@robertwb

This comment has been minimized.

@williamstein
Copy link
Contributor

comment:3

Note: 9051-FpT_2.patch is by David Roe.

@robertwb
Copy link
Contributor Author

Attachment: 9051-FpT_4.patch.gz

@williamstein
Copy link
Contributor

flattened parts1-4 and rebased against sage-4.4.4

@williamstein
Copy link
Contributor

comment:4

Attachment: trac_9051-flattened_and_rebased.patch.gz

I took sage-4.4.4 and applied trac_9051-flattened_and_rebased.patch. Doctesting just rings/ fails very seriously after applying this patch:


sage -t devel/sage/sage/rings/

The following tests failed:

        sage -t  devel/sage-main/sage/rings/residue_field.pyx # 16 doctests failed
        sage -t  devel/sage-main/sage/rings/number_field/order.py # 3 doctests failed
        sage -t  devel/sage-main/sage/rings/number_field/number_field_element_quadratic.pyx # 2 doctests failed
        sage -t  devel/sage-main/sage/rings/arith.py # 1 doctests failed
        sage -t  devel/sage-main/sage/rings/ring.pyx # 5 doctests failed
        sage -t  devel/sage-main/sage/rings/polynomial/multi_polynomial.pyx # 2 doctests failed
        sage -t  devel/sage-main/sage/rings/polynomial/multi_polynomial_libsingular.pyx # 7 doctests failed
        sage -t  devel/sage-main/sage/rings/integer_ring.pyx # 5 doctests failed
        sage -t  devel/sage-main/sage/rings/number_field/galois_group.py # 8 doctests failed
        sage -t  devel/sage-main/sage/rings/polynomial/polynomial_element.pyx # 7 doctests failed
        sage -t  devel/sage-main/sage/rings/misc.py # 1 doctests failed
        sage -t  devel/sage-main/sage/rings/number_field/number_field_base.pyx # 13 doctests failed
        sage -t  devel/sage-main/sage/rings/qqbar.py # 4 doctests failed
        sage -t  devel/sage-main/sage/rings/number_field/number_field_rel.py # 10 doctests failed
        sage -t  devel/sage-main/sage/rings/polynomial/multi_polynomial_ideal.py # 15 doctests failed
        sage -t  devel/sage-main/sage/rings/polynomial/polynomial_singular_interface.py # 1 doctests failed
        sage -t  devel/sage-main/sage/rings/number_field/number_field_element.pyx # 13 doctests failed
----------------------------------------------------------------------
Timings have been updated.
Total time for all tests: 64.0 seconds

@roed314
Copy link
Contributor

roed314 commented Jul 9, 2010

Fixes the broken doctests

@roed314
Copy link
Contributor

roed314 commented Jul 9, 2010

comment:5

Attachment: trac_9051_polycall_fixes.patch.gz

@roed314
Copy link
Contributor

roed314 commented Jul 9, 2010

comment:6

The most recent patch should be applied on top of the flattened and rebased patche.

@williamstein
Copy link
Contributor

comment:7

I'm running tests with both patches:

http://sage.math.washington.edu/home/wstein/build/sage-4.4.4/devel/sage/9051.out

@williamstein
Copy link
Contributor

comment:8

Stuff fails. See above link.

@williamstein
Copy link
Contributor

comment:9
----------------------------------------------------------------------

The following tests failed:

        sage -t  devel/sage-main/sage/modular/abvar/homspace.py # 20 doctests failed
        sage -t  devel/sage-main/sage/modular/abvar/abvar.py # 32 doctests failed
        sage -t  devel/sage-main/sage/modular/modsym/space.py # 6 doctests failed
        sage -t  devel/sage-main/sage/modular/modsym/ambient.py # 2 doctests failed
        sage -t  devel/sage-main/sage/modular/modform/element.py # 2 doctests failed
        sage -t  devel/sage-main/sage/functions/piecewise.py # 6 doctests failed
        sage -t  devel/sage-main/sage/graphs/graph.py # 1 doctests failed
        sage -t  devel/sage-main/sage/modular/abvar/morphism.py # 3 doctests failed
        sage -t  devel/sage-main/sage/modular/abvar/abvar_ambient_jacobian.py # 3 doctests failed
        sage -t  devel/sage-main/sage/modular/abvar/homology.py # 19 doctests failed
        sage -t  devel/sage-main/sage/modular/abvar/abvar_newform.py # 12 doctests failed
        sage -t  devel/sage-main/sage/tests/startup.py # 1 doctests failed
        sage -t  devel/sage-main/sage/numerical/optimize.py # 2 doctests failed
        sage -t  devel/sage-main/sage/modular/abvar/constructor.py # 2 doctests failed
        sage -t  devel/sage-main/sage/schemes/hyperelliptic_curves/hypellfrob.pyx # 1 doctests failed
----------------------------------------------------------------------
Timings have been updated.
Total time for all tests: 355.5 seconds
                                                            

@roed314
Copy link
Contributor

roed314 commented Jul 9, 2010

Attachment: trac_9051_fixes2.patch.gz

Fixes more doctests

@roed314
Copy link
Contributor

roed314 commented Jul 9, 2010

comment:10

Thanks; I was going to run tests while sleeping, but this worked better. I think I have them all now, but I haven't run tests after the fix: I'm doing it on my laptop, so it'll take a while.

@williamstein
Copy link
Contributor

@roed314
Copy link
Contributor

roed314 commented Jul 9, 2010

comment:12

Looks like all tests pass; do you want to review it?

@williamstein
Copy link
Contributor

comment:13

Wow, that's excellent that everything finally passes. Yes, I hope to have time to review it soon.

@williamstein
Copy link
Contributor

comment:14

Attachment: trac_9051-everything_flattened.patch.gz

I did a benchmark on sage.math, comparing this code to Magma:

SAGE with your patch:

sage: R.<T> = GF(71)[]; K.<T> = FractionField(R); a=(T^3+T-1)*(T^2-2); b=(3*T^5-T+7)*(T^2-2)
sage: timeit('a/b+b/a')
625 loops, best of 3: 26.3 µs per loop
sage: time v=[a/b+b/a for i in range(10^5)]
CPU times: user 2.94 s, sys: 0.02 s, total: 2.96 s
Wall time: 2.96 s
sage: time v=[a*b for i in range(10^5)]
CPU times: user 0.54 s, sys: 0.02 s, total: 0.56 s
Wall time: 0.56 s
sage: time v=[(1/a)*(1/b) for i in range(10^5)]
CPU times: user 1.80 s, sys: 0.00 s, total: 1.80 s
Wall time: 1.80 s

Before the patch, the same benchmark is massively slower, so this patch is a very big improvement:

sage: sage: R.<T> = GF(71)[]; K.<T> = FractionField(R); a=(T^3+T-1)*(T^2-2); b=(3*T^5-T+7)*(T^2-2)
sage: sage: timeit('a/b+b/a')
625 loops, best of 3: 776 µs per loop

In Magma:

sage: R.<T> = GF(71)[]; K.<T> = FractionField(R); a=(T^3+T-1)*(T^2-2); b=(3*T^5-T+7)*(T^2-2)
sage: aa=magma(a); bb=magma(b)
sage: magma.eval('a:=%s;b:=%s;'%(aa.name(),bb.name()))
sage: magma.eval('time v := [a/b+b/a : i in [1..10^5]];')
'Time: 0.800'
sage: magma.eval('time v := [a*b : i in [1..10^5]];')
'Time: 0.320'
sage: magma.eval('time v := [(1/a) * (1/b) : i in [1..10^5]];')
'Time: 0.830'

Something surprising is that working in your rational function field is much faster than working with polynomials!

sage: R.<T> = GF(71)[];  a=(T^3+T-1)*(T^2-2); b=(3*T^5-T+7)*(T^2-2)
sage: time v=[a*b for i in range(10^5)]
CPU times: user 2.02 s, sys: 0.00 s, total: 2.02 s
Wall time: 2.02 s

@williamstein
Copy link
Contributor

comment:15

Before the patch... 79 seconds instead of the new 2.9 seconds:


sage: sage: time v=[a/b+b/a for i in range(10^5)]
CPU times: user 78.91 s, sys: 0.15 s, total: 79.06 s
Wall time: 79.05 s

@williamstein
Copy link
Contributor

Attachment: trac_9051-referee-1.patch.gz

apply this after the "everything flattened" patch directly above.

@robertwb
Copy link
Contributor Author

comment:17

Reviewer patch looks good to me. My only comment is that it would be nice to have a faster not-equals comparison, but that's not worth holding this up.

@qed777
Copy link
Mannequin

qed777 mannequin commented Jul 20, 2010

comment:19

I've merged only

in 4.5.2.alpha0. Please correct the Author(s) and Reviewer(s) fields, if I'm wrong.

@qed777
Copy link
Mannequin

qed777 mannequin commented Jul 20, 2010

Merged: sage-4.5.2.alpha0

@qed777
Copy link
Mannequin

qed777 mannequin commented Jul 20, 2010

Reviewer: Robert Bradshaw, William Stein

@qed777
Copy link
Mannequin

qed777 mannequin commented Jul 20, 2010

Author: Robert Bradshaw, David Roe, William Stein

@qed777 qed777 mannequin removed the s: positive review label Jul 20, 2010
@qed777 qed777 mannequin closed this as completed Jul 20, 2010
@jdemeyer
Copy link
Contributor

comment:20

I assume that it's a mistake that the function

def fraction_field(self):

was added twice in sage/rings/polynomial/polynomial_ring.py

@jdemeyer
Copy link
Contributor

comment:21

Please review #13971 to correct this duplicate method.

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

5 participants