Add `IsPrime` checker to `math` package #3738

rhagenson · 2021-04-03T21:00:19Z

Due to some work on roaring-pony which proved to me above my head at this time, I implemented an IsPrime checker which might be useful for folks in the future. I am currently working within community members over on Zulip to learn enough LLVM IR to determine if the pre-allocated array of primes is being statically allocated as I intend.

rhagenson · 2021-04-03T23:46:13Z

Following work by @jemc this now uses a statically allocated table of primes, built via a match table.

I also fixed a bug where 1 was being called a prime, which it is not.

ponylang-main · 2021-04-04T00:03:31Z

Hi @rhagenson,

The changelog - added label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do.

Release notes are added by creating a uniquely named file in the .release-notes directory. We suggest you call the file 3738.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

SeanTAllen · 2021-04-04T00:04:22Z

packages/math/is_prime.pony

+    match index
+    | 0 => 2 | 1 => 3 | 2 => 5 | 3 => 7 | 4 => 11 
+    | 5 => 13 | 6 => 17 | 7 => 19 | 8 => 23 | 9 => 29 
+    | 10 => 31 | 11 => 37 | 12 => 41 | 13 => 43 | 14 => 47 
+    | 15 => 53 | 16 => 59 | 17 => 61 | 18 => 67 | 19 => 71 
+    | 20 => 73 | 21 => 79 | 22 => 83 | 23 => 89 | 24 => 97
+    | 25 => 101 | 26 => 103 | 27 => 107 | 28 => 109 | 29 => 113
+    | 30 => 127 | 31 => 131 | 32 => 137 | 33 => 139 | 34 => 149
+    | 35 => 151 | 36 => 157 | 37 => 163 | 38 => 167 | 39 => 173
+    | 40 => 179 | 41 => 181 | 42 => 191 | 43 => 193 | 44 => 197
+    | 45 => 199 | 46 => 211 | 47 => 223 | 48 => 227 | 49 => 229
+    | 50 => 233 | 51 => 239 | 52 => 241 | 53 => 251 | 54 => 257
+    | 55 => 263 | 56 => 269 | 57 => 271 | 58 => 277 | 59 => 281
+    | 60 => 283 | 61 => 293 | 62 => 307 | 63 => 311 | 64 => 313
+    | 65 => 317 | 66 => 331 | 67 => 337 | 68 => 347 | 69 => 349
+    | 70 => 353 | 71 => 359 | 72 => 367 | 73 => 373 | 74 => 379
+    | 75 => 383 | 76 => 389 | 77 => 397 | 78 => 401 | 79 => 409


@Theodus thoughts on this vis-a-vis the style guide?

Each branch on a separate line would be consistent with the rest of the stdlib. But a match of this size is unusual, so I don't have strong opinions about it.

SeanTAllen · 2021-04-04T00:05:37Z

packages/math/is_prime.pony

+  // Using a match table like below provides information to LLVM
+  // to allocate static globals rather than stack or heap allocation.
+  fun _low_prime_table_size(): USize => 256
+  fun _low_prime_table(index: USize): A val =>


There should be a newline between these methods.

SeanTAllen · 2021-04-04T00:06:49Z

packages/math/is_prime.pony

+  fun apply(n: A): Bool =>
+    var table_index: USize = 0
+    while table_index < _low_prime_table_size() do
+      let prime = _low_prime_table(table_index); table_index = table_index + 1


what's the reason for the ';' here?

How Joe had it written. It separates two statements on the same line. I have made them two lines locally and will push with my next commit.

I'm a dirty multi-statement line writer sometimes! I admit it!

SeanTAllen · 2021-04-04T00:07:28Z

packages/math/is_prime.pony

+      if n == prime then return true end
+      if (n %% prime) == 0 then return false end
+    end
+    if n == 1 then return false end  // 1 is not prime


My preference is for comments like the one at the end of the line to be before the line (i.e. above it rather than trailing)

I blend both trailing and line before comments depending on length of comment. No strong opinion on "right" so I will make the comment be on the previous line.

SeanTAllen · 2021-04-04T00:08:36Z

packages/math/is_prime.pony

@@ -0,0 +1,80 @@
+primitive IsPrime[A: (Integer[A] val & Unsigned)]
+  """Primality test using 6k+-1 optimization."""


Is there anything that can be linked to that explains this optimization?

I am trying to find a good source. My search landed me on Wikipedia, but I would prefer something more definitive.

The relevant passage is this, but has no source to it.

We can improve this method further. Observe that all primes greater than 3 are of the form 6k ± 1, where k is any integer greater than 0. This is because all integers can be expressed as (6k + i), where i = −1, 0, 1, 2, 3, or 4. Note that 2 divides (6k + 0), (6k + 2), and (6k + 4) and 3 divides (6k + 3). So, a more efficient method is to test whether n is divisible by 2 or 3, then to check through all numbers of the form 6 k ± 1 ≤ n {\displaystyle \scriptstyle 6k\ \pm \ 1\leq {\sqrt {n}}} {\displaystyle \scriptstyle 6k\ \pm \ 1\leq {\sqrt {n}}}. This is 3 times faster than testing all numbers up to √n.

Anything is better than nothing! (Maybe) 😉

SeanTAllen · 2021-04-04T00:09:46Z

packages/math/is_prime.pony

+  // Using a match table like below provides information to LLVM
+  // to allocate static globals rather than stack or heap allocation.


Should this comment be on the _low_primate_table rather than the size function here? Or perhaps right before the table itself?

I agree. It is why I had both methods without a newline as they were sort of acting as one method together. I moved the size method down below the table so the comment could be on top and be above the table method itself.

SeanTAllen · 2021-04-04T00:12:16Z

This is an awesome addition. Thanks @rhagenson.

rhagenson · 2021-04-04T00:32:47Z

@SeanTAllen I got all your comments addressed. I was not able to track down the source of the 6k±1 fact...each place I look simple states it as fact rather than stating where/how it was first formulated.

rhagenson · 2021-04-04T00:42:46Z

I figured out the 6k±1 connection. All integers can be written in as either a multiple of six (i.e., 6k+0) or as being between multiples of six (6k + i, where i is -1, 1, 2, 3, or 4) and because all, but two of those i choices create numbers that are divisible by either 2 or 3 then only those remaining two i choices can encode prime numbers.

SeanTAllen · 2021-04-04T00:43:20Z

.release-notes/3738.md

+## Add IsPrime to math package
+
+Quickly determine if a given number is prime using the 6k ± 1 method.
+
+All integers (excluding 2 and 3), can be expressed as (6k + i), where i = −1, 0, 1, 2, 3, or 4. Since 2 divides (6k + 0), (6k + 2), and (6k + 4), while 3 divides (6k + 3) that leaves only (6k - 1) and (6k + 1) for expressing primes


Nice. Given that these notes end up in markdown on GitHub. Feel free to do IsPrime and math is you want or to use other markdown formatting as well. (You don't have to though)

rhagenson · 2021-04-04T00:45:03Z

It does not have a source because it is (once you realize what it is saying) pretty obviously true. I had taken the 6k±1 to only apply to primes, but applies to all integers -- once I realized that it all fell into place on why it makes sense.

rhagenson · 2021-04-04T01:32:46Z

I am trying to think up a good way to add tests before this is merged. I could reasonable take the 6k+-i formula and verify any such k with i other than +1 and -1 is not prime. I need to put more thought into if that is a good way to test.

I also would want to check the various types are correct, ie the use of values inside the static table outside the range of U8 does not miscall when using IsPrime[U8].

rhagenson · 2021-04-04T03:59:03Z

As I suspected, the tests do not pass for all types. I will need to investigate further as to why. Failure on U8 I expected, but failing on U16 surprised me.

Edit: I saw these failures locally, but wanted to push the changes as I am done for the evening. I will see if they fail in CI as well.

rhagenson · 2021-04-04T04:13:12Z

I think the local failures may be due to some oddities on my local machine. I was playing around with debug builds of ponyc the other day and suspect a make distclean and re-build from source may be in order.

rhagenson · 2021-04-04T04:29:22Z

If the tests all pass, I am going to extend these tests so it is checking more than one value in each range. I will add it as a parameter at the test builder level so we can use it as _IsPrimeTestBuilder[U128]("U128", 100) to check 100 random U128 values.

SeanTAllen · 2021-04-04T04:37:49Z

@rhagenson if your PR isn't ready to be merged, go ahead and add a "DO NOT MERGE" label to it so that no one accidentally merges early. And whenever you are ready, go ahead and do a "Squash and merge". Just remember to update the commit comment when squashing.

rhagenson · 2021-04-04T17:58:35Z

If the tests all pass, I am going to extend these tests so it is checking more than one value in each range. I will add it as a parameter at the test builder level so we can use it as _IsPrimeTestBuilder[U128]("U128", 100) to check 100 random U128 values.

I figured out the reason tests were passing is because stdlib/_test.pony did not actually run the (prior to now absent) tests in math. I also decided against adding a parameter in favor of basing the number of values to check on two times the bitwidth so U8 checks 16 values, U16 checks 32 values, ..., U128 checks 256 values. Less arbitrary and makes the number of values checked scale with the number of values possible.

Edit: Added part about basing number of values to check on bitwidth.

SeanTAllen · 2021-04-04T18:06:13Z

packages/stdlib/_test.pony

@@ -63,6 +63,7 @@ actor Main is TestList
    itertools.Main.make().tests(test)
    json.Main.make().tests(test)
    logger.Main.make().tests(test)
+    math.Main.make().tests(test)


d'oh! I thought this already existed. dur. I should have realized that it didn't based on the rest of the PR.

Sorry about that.

No worries. Before now, math had no tests so it made sense that this line was not present.

rhagenson · 2021-04-05T21:38:16Z

@SeanTAllen Before I squash and merge this, as it is my first such commit to the ponyc repo, any advice/requirements to follow for a good commit message?

SeanTAllen · 2021-04-05T21:47:34Z

No advice beyond what is in the contributing guide.

rhagenson · 2021-04-05T21:52:32Z

Got it. I will give that another read then merge this.

rhagenson added 3 commits April 3, 2021 15:56

Provide class-based IsPrime checker

b9b95fd

Don't check n %% 2 and n %% 3 twice

e64e4da

Static allocation via match table

c259b45

Fix later primes in match table

f1d6212

SeanTAllen added the changelog - added Automatically add "Added" CHANGELOG entry on merge label Apr 4, 2021

SeanTAllen reviewed Apr 4, 2021

View reviewed changes

SeanTAllen changed the title ~~Provide class-based IsPrime checker~~ Add IsPrime checker to Math package Apr 4, 2021

SeanTAllen reviewed Apr 4, 2021

View reviewed changes

rhagenson added 6 commits April 3, 2021 19:12

Rm unneccesary newline

dea9491

Re-order methods

f893739

Put expressions on different lines

e95bb1e

Comment on line above

70cb1ea

Docstring explaining method

81a2e85

Release notes with method explaination.

f48ff06

Clarify methodology

77689f2

SeanTAllen reviewed Apr 4, 2021

View reviewed changes

Update with Markdown formatting

c94da8d

rhagenson marked this pull request as ready for review April 4, 2021 00:47

rhagenson changed the title ~~Add IsPrime checker to Math package~~ Add IsPrime checker to math package Apr 4, 2021

Add tests

3f34298

rhagenson added the do not merge This PR should not be merged at this time label Apr 4, 2021

rhagenson mentioned this pull request Apr 4, 2021

Last Week in Pony - April 4, 2021 ponylang/ponylang-website#656

Closed

Add math tests and fix U8 primality

2ce4d7a

SeanTAllen reviewed Apr 4, 2021

View reviewed changes

Specify UnsignedInteger; test ULong and USize

9e71e63

rhagenson removed the do not merge This PR should not be merged at this time label Apr 5, 2021

rhagenson merged commit ae556fe into main Apr 5, 2021

rhagenson deleted the add-is-prime branch April 5, 2021 22:07

github-actions bot pushed a commit that referenced this pull request Apr 5, 2021

Update CHANGELOG for PR #3738

b5bbc9a

github-actions bot pushed a commit that referenced this pull request Apr 5, 2021

Updates release notes for PR #3738

17167f1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `IsPrime` checker to `math` package #3738

Add `IsPrime` checker to `math` package #3738

rhagenson commented Apr 3, 2021

rhagenson commented Apr 3, 2021 •

edited

Loading

ponylang-main commented Apr 4, 2021

SeanTAllen Apr 4, 2021

Theodus Apr 4, 2021

SeanTAllen Apr 4, 2021

SeanTAllen Apr 4, 2021

rhagenson Apr 4, 2021

jemc Apr 4, 2021

SeanTAllen Apr 4, 2021

rhagenson Apr 4, 2021

SeanTAllen Apr 4, 2021

rhagenson Apr 4, 2021

rhagenson Apr 4, 2021

SeanTAllen Apr 4, 2021

SeanTAllen Apr 4, 2021

rhagenson Apr 4, 2021

SeanTAllen commented Apr 4, 2021

rhagenson commented Apr 4, 2021

rhagenson commented Apr 4, 2021

SeanTAllen Apr 4, 2021

rhagenson Apr 4, 2021

rhagenson commented Apr 4, 2021

rhagenson commented Apr 4, 2021 •

edited

Loading

rhagenson commented Apr 4, 2021 •

edited

Loading

rhagenson commented Apr 4, 2021

rhagenson commented Apr 4, 2021

SeanTAllen commented Apr 4, 2021

rhagenson commented Apr 4, 2021 •

edited

Loading

SeanTAllen Apr 4, 2021

rhagenson Apr 4, 2021

rhagenson commented Apr 5, 2021

SeanTAllen commented Apr 5, 2021

rhagenson commented Apr 5, 2021

		@@ -0,0 +1,80 @@
		primitive IsPrime[A: (Integer[A] val & Unsigned)]
		"""Primality test using 6k+-1 optimization."""

		// Using a match table like below provides information to LLVM
		// to allocate static globals rather than stack or heap allocation.

Add IsPrime checker to math package #3738

Add IsPrime checker to math package #3738

Conversation

rhagenson commented Apr 3, 2021

rhagenson commented Apr 3, 2021 • edited Loading

ponylang-main commented Apr 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanTAllen commented Apr 4, 2021

rhagenson commented Apr 4, 2021

rhagenson commented Apr 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhagenson commented Apr 4, 2021

rhagenson commented Apr 4, 2021 • edited Loading

rhagenson commented Apr 4, 2021 • edited Loading

rhagenson commented Apr 4, 2021

rhagenson commented Apr 4, 2021

SeanTAllen commented Apr 4, 2021

rhagenson commented Apr 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhagenson commented Apr 5, 2021

SeanTAllen commented Apr 5, 2021

rhagenson commented Apr 5, 2021

Add `IsPrime` checker to `math` package #3738

Add `IsPrime` checker to `math` package #3738

rhagenson commented Apr 3, 2021 •

edited

Loading

rhagenson commented Apr 4, 2021 •

edited

Loading

rhagenson commented Apr 4, 2021 •

edited

Loading

rhagenson commented Apr 4, 2021 •

edited

Loading