-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
fixed is_nthpow_residue #18398
fixed is_nthpow_residue #18398
Conversation
✅ Hi, I am the SymPy bot (v149). 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:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.6. 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.
Update The release notes on the wiki have been updated. |
Codecov Report
@@ Coverage Diff @@
## master #18398 +/- ##
=============================================
+ Coverage 75.292% 75.304% +0.011%
=============================================
Files 633 637 +4
Lines 166944 167071 +127
Branches 39396 39417 +21
=============================================
+ Hits 125697 125812 +115
- Misses 35709 35719 +10
- Partials 5538 5540 +2 |
sympy/ntheory/residue_ntheory.py
Outdated
@@ -1533,6 +1532,8 @@ def polynomial_congruence(expr, m): | |||
if rank == 2: | |||
return quadratic_congruence(0, coefficients[0], coefficients[1], | |||
m) | |||
if coefficients[0] == 1 and all(v == 0 for v in coefficients[1:-1]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if coefficients[0] == 1 and all(v == 0 for v in coefficients[1:-1]): | |
if coefficients[0] == 1 and 1 + coefficients[-1] = sum(coefficients): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the coefficient is negative this might give wrong result. I think I should take mod m of all coefficients then this suggestion can be correctly implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, what you have is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why not
if not any(coefficients[1: -1]):
g = igcd(coefficients[0], coefficients[-1])
if coefficients[0]//g == 1:
return nthroot_mod(-coefficients[-1]//g, rank - 1, m, True)
In that case 2*x**2 + 8
will be reduced to x**2 + 4
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smichr yes but in this case also we have to take mod m
of the coefficients. For example:
for x**5 + 16 x + 7 = 0 mod 8
. But this can be reduced to x**5 + 7
. But (if not any(coefficients[1:-1])
) will not catch this unless coefficients = coefficients mod m
.
Also by dividing the coefficients[0] with gcd
might result in loss of some roots.
For example : 2*x**2 + 8 = 0 mod 10
has roots [1,4,6,9] but x**2 + 4 = 0 mod 10
has
roots [4,6] because mod_inv(2,10)
does not exist.
I don't know whether the changes here are good but the release note can be improved. Try to write a release note that makes sense for end users. They don't care about internal details: they just want know what has changed from the outside. You can see examples of release notes here: |
I edited release notes. |
Fixes #18394
is_nthpow_residue
no longer raises ValueError when a < 0polynomial_congruence
recognizes x**n + a = 0 mod m as a special case