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

Fix the calculation of "is prime" for numbers after 1321. #3799

Merged
merged 3 commits into from
Jul 13, 2021
Merged

Conversation

SeanTAllen
Copy link
Member

No description provided.

@SeanTAllen SeanTAllen added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Jul 12, 2021
@SeanTAllen SeanTAllen requested a review from a team July 12, 2021 21:24
@EpicEric
Copy link
Contributor

Should this have regression tests?

One possibility to cover a wide range of values is to test for U128 Mersenne primes, for example:

class MersennePrimeTest is UnitTest
  fun name(): String => "math/MersennePrimeTest"

  fun apply(h: TestHelper) =>
    let mersenne_prime_exponents: Array[U8] = [
      2; 3; 5; 7
      13; 17; 19; 31
      61; 89; 107; 127
    ]
    var p: U8 = 128
    var x = U128.max_value()  // (2 ^ p) - 1
    while p >= 2 do
      if mersenne_prime_exponents.contains(p) then
        h.assert_true(IsPrime[U128](x))
      else
        h.assert_false(IsPrime[U128](x))
      end
      p = p - 1
      x = x >> 1
    end

@rhagenson
Copy link
Member

Eric, I am always for more tests covering a wider range. I am not familiar with Mersenne primes though.

I am going to adding tests to this PR before it is merged and reduce the redundancy in the implementation.

@rhagenson
Copy link
Member

I am working on my additions here.

I am currently looking at generalizing Mersenne prime production.

@SeanTAllen
Copy link
Member Author

@rhagenson if the additional tests aren't ready in the next couple days, let's merge this so it goes out in the release and go from there.

@rhagenson
Copy link
Member

rhagenson commented Jul 13, 2021

Sounds good. I will push what I have done today (short of any WIP stuff) so we can get it ready. The issue right now is that Mersenne primes are computationally intensive so I am trying to find a way to pare that down slightly to test a large portion of the range, but not all.

@rhagenson
Copy link
Member

rhagenson commented Jul 13, 2021

The only problematic type is U128 because the calculations done within IsPrime increment upwards by 6 while the test is checking for the primality of (2 ^ 127) - 1. This takes a long, long time. Until we can find a way to shortcut this, I am okay with not having a Mersenne prime test for U128.

@rhagenson
Copy link
Member

The force push is me using git fixup for the first time (I just learned about it last week thanks to Sean) so if it broke something I will need help fixing it. I had neglected to remove some debugging stuff, including the use "debug" itself.

@rhagenson
Copy link
Member

I have going to remove myself as a reviewer since obviously at this time I have made additions so there is a general implicit approval.

Copy link
Member

@rhagenson rhagenson left a comment

Choose a reason for hiding this comment

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

Could not quickly determine how to unassign myself so going to stamp it with my approval anyhow as I have to switch what I am focusing on right now.

@SeanTAllen SeanTAllen merged commit 4197988 into main Jul 13, 2021
@SeanTAllen SeanTAllen deleted the prime-fix branch July 13, 2021 02:32
github-actions bot pushed a commit that referenced this pull request Jul 13, 2021
github-actions bot pushed a commit that referenced this pull request Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants