Updated nthroot to raise NotImplementedError for composite modulo #18194
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. |
This is the output on my terminal This is the test which failed the last time. |
|
Primitive root in sympy can compute for composite number. |
sympy/ntheory/residue_ntheory.py
Outdated
| @@ -773,6 +777,8 @@ def nthroot_mod(a, n, p, all_roots=False): | |||
| # see Hackman "Elementary Number Theory" (2009), page 76 | |||
| if not is_nthpow_residue(a, n, p): | |||
| return None | |||
| if isprime(p) is False: | |||
There was a problem hiding this comment.
It seems that isprime() will return True or False. Hence this could be if not isprime(p):.
sympy/ntheory/residue_ntheory.py
Outdated
| @@ -773,6 +777,8 @@ def nthroot_mod(a, n, p, all_roots=False): | |||
| # see Hackman "Elementary Number Theory" (2009), page 76 | |||
| if not is_nthpow_residue(a, n, p): | |||
| return None | |||
| if isprime(p) is False: | |||
| raise NotImplementedError("Not implemented for composite p") | |||
| if primitive_root(p) is None: | |||
| raise NotImplementedError("Not Implemented for m without primitive root") | |||
There was a problem hiding this comment.
If p is prime, then a primitive root exists. So these two lines can be removed.
There was a problem hiding this comment.
But we have to add a primality test for the modulo right. Without it we were getting wrong answers.
There was a problem hiding this comment.
The primality test on line 776 is currently necessary, but if p is prime, then a primitive root exists and the test on line 778 will always fail. So the two lines 778-779 can be removed.
sympy/ntheory/residue_ntheory.py
Outdated
| @@ -72,6 +72,8 @@ def _primitive_root_prime_iter(p): | |||
|
|
|||
| """ | |||
| # it is assumed that p is an int | |||
| if isprime(p) is False: | |||
| raise NotImplementedError("Not implemented for composite p") | |||
There was a problem hiding this comment.
This test is not necessary as this is a private method and 'prime' is mentioned.
sympy/ntheory/residue_ntheory.py
Outdated
| @@ -110,6 +112,8 @@ def primitive_root(p): | |||
| p = as_int(p) | |||
| if p < 1: | |||
| raise ValueError('p is required to be positive') | |||
| if isprime(p) is False: | |||
| raise NotImplementedError("Not implemented for composite p") | |||
There was a problem hiding this comment.
Primitive roots exist also for some composite moduli (e.g. odd prime powers) and the implementation below takes care of those cases as well. So these two lines should be removed.
Codecov Report
@@ Coverage Diff @@
## master #18194 +/- ##
============================================
+ Coverage 74.936% 74.967% +0.03%
============================================
Files 642 642
Lines 167128 167128
Branches 39331 39331
============================================
+ Hits 125240 125291 +51
+ Misses 36351 36303 -48
+ Partials 5537 5534 -3 |
|
Thank you. |
References to other Issues or PRs
#18172
Brief description of what is fixed or changed
nthroot_mod,primitive_rootand_primitive_root_prime_iterhave been updated to not work for composite modulo.Other comments
A more general algorithm to compute nthroot has to be implemented to work with all possible modulo
Release Notes
nthroot_mod,primitive_rootand_primitive_root_prime_iterincompatible with composite modulo.