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

Improvement_of_dup_zz_mignotte_bound(f, K) #19254

Merged
merged 14 commits into from May 8, 2020
33 changes: 27 additions & 6 deletions sympy/polys/factortools.py
Expand Up @@ -73,6 +73,7 @@

from sympy.ntheory import nextprime, isprime, factorint
from sympy.utilities import subsets
from sympy import binomial

from math import ceil as _ceil, log as _log

Expand Down Expand Up @@ -124,13 +125,33 @@ def dmp_trial_division(f, factors, u, K):


def dup_zz_mignotte_bound(f, K):
"""Mignotte bound for univariate polynomials in `K[x]`. """
a = dup_max_norm(f, K)
b = abs(dup_LC(f, K))
n = dup_degree(f)
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears to be a public function. So, Parameters and Examples sections can be added in the doc strings.

The Knuth-Cohen variant of Mignotte bound for
univariate polynomials in `K[x]`.

return K.sqrt(K(n + 1))*2**n*a*b
References
==========

..[1] [Abbott2013]_

"""
d = dup_degree(f)
delta = _ceil( d / 2 )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In SymPy, no space is added after ( and before ). So this could be _ceil(d/2).


# euclidean-norm
eucl_norm = K.sqrt( sum( [cf**2 for cf in f] ) )

# biggest values of binomial coefficients (p. 538 of reference)
t1 = binomial( delta - 1, _ceil( delta / 2 ) )
lagamura marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would consider defining something like delta2 = _ceil(delta/2) in addition to delta and using it on this and the next line.

t2 = binomial( delta - 1, _ceil( delta / 2 ) - 1 )

lc = abs( dup_LC(f, K) ) # leading coefficient
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In principle, this could probably have K.abs instead of abs.


bound = t1 * eucl_norm + t2 * lc # (p. 538 of reference)

bound = _ceil( bound / 2 ) * 2 # round up to even integer

return bound + dup_max_norm(f, K) # add max_coeff for irreducible polys

def dmp_zz_mignotte_bound(f, u, K):
"""Mignotte bound for multivariate polynomials in `K[X]`. """
Expand Down Expand Up @@ -1147,7 +1168,7 @@ def dmp_ext_factor(f, u, K):
return lc, []

f, F = dmp_sqf_part(f, u, K), f
s, g, r = dmp_sqf_norm(F, u, K)
s, g, r = dmp_sqf_norm(f, u, K)

factors = dmp_factor_list_include(r, u, K.dom)

Expand Down
2 changes: 1 addition & 1 deletion sympy/polys/tests/test_factortools.py
Expand Up @@ -27,7 +27,7 @@ def test_dmp_trial_division():

def test_dup_zz_mignotte_bound():
R, x = ring("x", ZZ)
assert R.dup_zz_mignotte_bound(2*x**2 + 3*x + 4) == 32
assert R.dup_zz_mignotte_bound(2*x**2 + 3*x + 4) == 6
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps there could be a couple of more examples, preferable with comments on actual bounds for comparison.



def test_dmp_zz_mignotte_bound():
Expand Down