Improvement_of_dup_zz_mignotte_bound(f, K) #19254

lagamura · 2020-05-04T13:38:13Z

So, this is our change in the dup_zz_mignotte_bound(f, K) method. Also, we have replace the old value in the test_factortools.py file. If it is asked, we can cite our test_file with the comparison of the bounds. As it was mentioned, we have not create a new method but just change the code in the dup_zz_mignotte_bound(f, K).

Fixes #19253

References to other Issues or PRs

Brief description of what is fixed or changed

dup_zz_mignotte_bound(f, K) implemented with Knuth-Cohen

Other comments

As it was mentioned in the issue, dmp_zz_mignotte_bound(f, u, K) should also be cared of.

Release Notes

polys
- improvement of dup_zz_mignotte_bound(f, K) by Knuth-Cohen bound

So, this is our change in the `dup_zz_mignotte_bound(f, K)` method. Also, we have replace the old value in the test_factortools.py file. If it is asked, we can cite our test_file with the comparison of the bounds.

sympy-bot · 2020-05-04T13:38:21Z

✅

Hi, I am the SymPy bot (v158). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

polys
- improvement of dup_zz_mignotte_bound(f, K) by Knuth-Cohen bound (#19254 by @lagamura)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.7.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

So, this is our change in the `dup_zz_mignotte_bound(f, K)` method. Also, we have replace the old value in the test_factortools.py file. If it is asked, we can cite our test_file with the comparison of the bounds. As it was mentioned, we have not create a new method but just change the code in the `dup_zz_mignotte_bound(f, K)`.


Fixes #19253 
<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->


#### Brief description of what is fixed or changed
`dup_zz_mignotte_bound(f, K)` implemented with Knuth-Cohen

#### Other comments
As it was mentioned in the issue, `dmp_zz_mignotte_bound(f, u, K)` should also be cared of.


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* polys
    * improvement of `dup_zz_mignotte_bound(f, K)` by Knuth-Cohen bound
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

sympy/polys/factortools.py

czgdp1807 · 2020-05-04T17:04:27Z

sympy/polys/factortools.py

-    n = dup_degree(f)
-
-    return K.sqrt(K(n + 1))*2**n*a*b
+    """


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

lagamura · 2020-05-04T19:12:17Z

Can someone help with the travis-ci test?

sympy/polys/factortools.py

divyanshu132 · 2020-05-04T19:52:46Z

Can someone help with the travis-ci test?

You can click on details to know about the failures!

lagamura · 2020-05-05T06:29:52Z

Managed to fix the basic errors of travis ci test. But now a bit more complex one occurred. Can anyone help with this?

asmeurer · 2020-05-05T21:33:23Z

This is the test failure

>>> expand(factor(expand(
...         (x - I*y)*(z - I*t)), extension=[I]))
x - I*y

As you can see, the answer is wrong (it should be -I*t*x - t*y + x*z - I*y*z, as in the test). So you will need to dig into the code to see what is happening.

correction on dmp_sqf_norm

codecov · 2020-05-06T08:52:33Z

Codecov Report

Merging #19254 into master will decrease coverage by 0.045%.
The diff coverage is 100.000%.

@@              Coverage Diff              @@
##            master    #19254       +/-   ##
=============================================
- Coverage   75.667%   75.622%   -0.046%     
=============================================
  Files          651       651               
  Lines       169487    169466       -21     
  Branches     40021     40014        -7     
=============================================
- Hits        128247    128154       -93     
- Misses       35623     35707       +84     
+ Partials      5617      5605       -12

jksuom · 2020-05-07T09:50:06Z

sympy/polys/factortools.py

+    from sympy import binomial
+
+    d = dup_degree(f)
+    delta = _ceil( d / 2 )


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

jksuom · 2020-05-07T09:50:38Z

sympy/polys/factortools.py

+    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 ) )


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

jksuom · 2020-05-07T09:54:25Z

sympy/polys/factortools.py

+    t1 = binomial( delta - 1, _ceil( delta / 2 ) )
+    t2 = binomial( delta - 1, _ceil( delta / 2 ) - 1 )
+
+    lc = abs( dup_LC(f, K) )   # leading coefficient


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

jksuom · 2020-05-07T09:55:18Z

sympy/polys/factortools.py

+    t2 = binomial( delta - 1, _ceil( delta / 2 ) - 1 )
+
+    lc = abs( dup_LC(f, K) )   # leading coefficient
+


This, and the next two blank lines could be removed.

jksuom · 2020-05-07T09:57:06Z

sympy/polys/tests/test_factortools.py

@@ -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


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

jksuom · 2020-05-07T09:58:52Z

I have some comments but otherwise this looks good.

Two examples also has been imported. I have not imported more as i am not so experienced in sympy's typing rules and mechanics. Your feedback is welcomed

One more example highlighting the difference between the old and the new method.

jksuom · 2020-05-08T06:54:54Z

Thanks, this is ready to go.

19253_Replacement_of_dup_zz_mignotte_bound(f, K)

8adfdbe

So, this is our change in the `dup_zz_mignotte_bound(f, K)` method. Also, we have replace the old value in the test_factortools.py file. If it is asked, we can cite our test_file with the comparison of the bounds.

divyanshu132 reviewed May 4, 2020

View reviewed changes

sympy/polys/factortools.py Outdated Show resolved Hide resolved

czgdp1807 reviewed May 4, 2020

View reviewed changes

czgdp1807 added the polys label May 4, 2020

divyanshu132 reviewed May 4, 2020

View reviewed changes

sympy/polys/factortools.py Outdated Show resolved Hide resolved

sympy/polys/factortools.py Outdated Show resolved Hide resolved

lagamura added 7 commits May 4, 2020 23:59

import binomial

68f7405

fixed trailing whitespace

cfffcd1

fixed References properly

c9aeae8

Update factortools.py

2a4c9dd

Update factortools.py

1daa06f

Update factortools.py

5332a98

Update factortools.py

9509193

lagamura added 2 commits May 6, 2020 01:20

bound now is even

9bb4c81

dmp_sqf_norm typo mistake

eaf5403

correction on dmp_sqf_norm

jksuom reviewed May 7, 2020

View reviewed changes

lagamura added 4 commits May 7, 2020 16:42

correction on sympy's principles and some examples

f5c9620

Two examples also has been imported. I have not imported more as i am not so experienced in sympy's typing rules and mechanics. Your feedback is welcomed

One more example

6f60e90

One more example highlighting the difference between the old and the new method.

trailing space fixed

49a41e3

small typing details

ad5638b

jksuom merged commit 2c3eb54 into sympy:master May 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvement_of_dup_zz_mignotte_bound(f, K) #19254

Improvement_of_dup_zz_mignotte_bound(f, K) #19254

lagamura commented May 4, 2020 •

edited by asmeurer

sympy-bot commented May 4, 2020 •

edited

czgdp1807 May 4, 2020

lagamura commented May 4, 2020

divyanshu132 commented May 4, 2020

lagamura commented May 5, 2020

asmeurer commented May 5, 2020

codecov bot commented May 6, 2020 •

edited

jksuom May 7, 2020

jksuom May 7, 2020

jksuom May 7, 2020

jksuom May 7, 2020

jksuom May 7, 2020

jksuom commented May 7, 2020

jksuom commented May 8, 2020

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

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

Improvement_of_dup_zz_mignotte_bound(f, K) #19254

Improvement_of_dup_zz_mignotte_bound(f, K) #19254

Conversation

lagamura commented May 4, 2020 • edited by asmeurer

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

sympy-bot commented May 4, 2020 • edited

czgdp1807 May 4, 2020

Choose a reason for hiding this comment

lagamura commented May 4, 2020

divyanshu132 commented May 4, 2020

lagamura commented May 5, 2020

asmeurer commented May 5, 2020

codecov bot commented May 6, 2020 • edited

Codecov Report

jksuom May 7, 2020

Choose a reason for hiding this comment

jksuom May 7, 2020

Choose a reason for hiding this comment

jksuom May 7, 2020

Choose a reason for hiding this comment

jksuom May 7, 2020

Choose a reason for hiding this comment

jksuom May 7, 2020

Choose a reason for hiding this comment

jksuom commented May 7, 2020

jksuom commented May 8, 2020

lagamura commented May 4, 2020 •

edited by asmeurer

sympy-bot commented May 4, 2020 •

edited

codecov bot commented May 6, 2020 •

edited