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

Implemented Quadratic residue function #736

Merged
merged 26 commits into from Mar 23, 2016

Conversation

CodeMaxx
Copy link
Contributor

@CodeMaxx CodeMaxx commented Jan 2, 2016

  • Quadratic Residue function
  • is_quad_residue function
  • is_nthroot_mod_prime_power function
  • is_nthroot_mod1() function
  • is_nth_power_residue() function

@isuruf @Sumith1896 Could you please review this?

@isuruf
Copy link
Member

isuruf commented Jan 2, 2016

There are compile errors. You can test locally by running make and then test using ctest -V.
Also, you can check here https://travis-ci.org/CodeMaxx/symengine

@CodeMaxx
Copy link
Contributor Author

CodeMaxx commented Jan 2, 2016

@isuruf Compiler errors fixed and it passes the one test I set. I'll add more tests. By that time you can review the rest of the code.

@certik
Copy link
Contributor

certik commented Jan 3, 2016

You got a test failure on Appveyor:

test_quadratic_residue(): ntheory
-------------------------------------------------------------------------------
C:\projects\symengine\symengine\tests\ntheory\test_ntheory.cpp:640
...............................................................................

C:\projects\symengine\symengine\tests\ntheory\test_ntheory.cpp:666: FAILED:
  REQUIRE( quadratic_residue(*a100) == i100 )
with expansion:
  { 0, 1, 4, 9, 15, 16, 20, 24, 25, 28, 29, 35, 36, 40, 44, 49, 56, 60, 61, 63,
  64, 68, 69, 75, 76, 81, 83, 89, 96, 99 }
  ==
  { 0, 1, 4, 9, 16, 21, 24, 25, 29, 36, 41, 44, 49, 56, 61, 64, 69, 76, 81, 84,
  89, 96 }

===============================================================================
test cases:  25 |  24 passed | 1 failed
assertions: 230 | 229 passed | 1 failed


      Start 14: test_diophantine

Can you look into that? Is that a bug in your code?

@@ -169,6 +169,9 @@ bool powermod(const Ptr<RCP<const Integer>> &powm, const RCP<const Integer> &a,
//! All solutions to x**s == a**r mod m where b = r / s. Return false if none exists.
void powermod_list(std::vector<RCP<const Integer>> &pows, const RCP<const Integer> &a,
const RCP<const Number> &b, const RCP<const Integer> &m);

//! Finds all Quadratic Residue of a Positive Integer
Copy link
Contributor

Choose a reason for hiding this comment

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

Residue -> Residues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected.

@CodeMaxx
Copy link
Contributor Author

CodeMaxx commented Jan 3, 2016

To see if it works I changed all of the ints to mpz_class

@CodeMaxx
Copy link
Contributor Author

CodeMaxx commented Jan 3, 2016

@certik Its working with mpz_class. But I still don't beleive it was an int overflow. The largest number in that test case was just 2500.

@isuruf
Copy link
Member

isuruf commented Jan 3, 2016

You were using (int) pow(i, 2) instead of just i*i. Casting double to int leads to incorrect results.

@CodeMaxx
Copy link
Contributor Author

CodeMaxx commented Jan 3, 2016

@isuruf But pow(i,2) always gave an integral answer that too <=2500 .

@isuruf
Copy link
Member

isuruf commented Jan 5, 2016

@CodeMaxx, the compiler is free to inline pow(i, 2) call to i*i when optimizations are on. That is why you get integral answers in Release mode.

pow(i, 2) is not inlined in Debug mode and in 32-bit MinGW, std::pow implementation returns an inexact answer. A compiler is not required to give the exact answer in floating point functions nor can a compiler represent numbers exactly.

You are right about overflow though, for 100, it is not possible to overflow.

@Sumith1896
Copy link
Contributor

@CodeMaxx Let us know if you need more help.

@CodeMaxx
Copy link
Contributor Author

CodeMaxx commented Jan 5, 2016

@Sumith1896 I'm having some trouble using mpz_class instead of int. Currently I'm pushing with some parts commented out which I can't get to work.

return false;

RCP<const Integer> x;
const RCP<const Integer> a1 = integer(a_final);
Copy link
Contributor

Choose a reason for hiding this comment

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

Take care of tabs and spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything is spaces. Is there an inconsistency anywhere?
Got it.

@Sumith1896
Copy link
Contributor

Can you add tests for the second function?

@CodeMaxx
Copy link
Contributor Author

CodeMaxx commented Jan 5, 2016

I'll add tests by tomorrow.
---------------------------append
or maybe in about an hour or something.

@@ -1194,6 +1194,23 @@ bool _nthroot_mod1(std::vector<RCP<const Integer>> &roots, const integer_class &
return true;
}

// Checks if Solution for x**n == a mod p**k exists where a != 0 mod p and p is an odd prime.
bool is_nthroot_mod1(const integer_class &a, const integer_class &n,
Copy link
Member

Choose a reason for hiding this comment

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

Rename to _is_nthroot_mod1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The functions not declared in the header files have names starting from _. Is that a symengine convention ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes

return is_nthroot_mod1(a, n, p, k);
}
} else {
integer_class _a;
Copy link
Member

Choose a reason for hiding this comment

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

See this from wikipedia

In general, to determine if a quadratic congruence with composite modulus is solvable use the following theorem:[37]

Let n > 1, and gcd(a,n) =1. Then x2 ≡ a (mod n) is solvable if and only if:

The Legendre symbol \left(\frac{a}{p}\right)=1 for all odd prime divisors p of n.
a ≡ 1 (mod 4) if 4|n, but 8 does not divide n; a ≡ 1 (mod 8) if 8|n.

Copy link
Member

Choose a reason for hiding this comment

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

Ignore that

@isuruf
Copy link
Member

isuruf commented Mar 10, 2016

@CodeMaxx, since the code is generic for nthroot, can you add a new method for nthroot as well?

@CodeMaxx
Copy link
Contributor Author

@isuruf Do you mean two functions? ( nth_residue and is_nth_residue)

@isuruf
Copy link
Member

isuruf commented Mar 10, 2016

Or just is_nth_residue.

@CodeMaxx
Copy link
Contributor Author

@isuruf I'm not sure if conditions like 1 and 2 hold for nth_residue or not. Do they?

@CodeMaxx
Copy link
Contributor Author

legendre is only for quadratic residues.

@isuruf So code is not generic for nthroot.

@CodeMaxx
Copy link
Contributor Author

@isuruf Can review this.

*/
{
if (mod->as_mpz() <= 0) {
return false;
Copy link
Member

Choose a reason for hiding this comment

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

mod can be negative

@isuruf
Copy link
Member

isuruf commented Mar 22, 2016

@CodeMaxx, I'm sorry I didn't see this message for review. I left a few comments. When you are done ping me.

@CodeMaxx
Copy link
Contributor Author

@isuruf Ping


bool is_nth_residue(const RCP<const Integer> &a, const RCP<const Integer> &n, const RCP<const Integer> &mod)
/*
Returns true if ``a`` (mod ``mod``) is in the set of squares mod ``mod``,
Copy link
Member

Choose a reason for hiding this comment

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

set of nth powers

@isuruf
Copy link
Member

isuruf commented Mar 22, 2016

+1 to merge when tests pass

@CodeMaxx
Copy link
Contributor Author

@isuruf ping

@isuruf isuruf merged commit 3912c27 into symengine:master Mar 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants