Added prev_prime(x) #284

sethtroisi · 2020-11-21T11:12:11Z

I authored mpz_prevprime and finally got it upstreamed. I'd love if gmpy2 would also support this (as I mostly call gmp via your wonderful wrapper).

Couple of notes

I throw Py_RETURN_NOTIMPLEMENTED if __GNU_MP_VERSION < 6.3
I'm not sure when gmp 6.3 release will happen but this is the safest option. I/you/others can always remove this line (or change it to 6.2) when using development version of the repo which uses build version 6.2.99.
mpz_prevprime has an int return (0 when no previous prime (arg < 2), definite prime, probable prime) I can return a two element tuple but IMHO it's better to just return the previous_prime (or 2).

casevh · 2020-11-22T03:33:18Z

Hi Seth,

Congratulations for getting prev_prime() accepted. I will merge this in a few days.

sethtroisi · 2020-11-24T21:44:25Z

I inverted the #if which fixes compilation. Tests are still broken because libgmp-dev is version 6.2
Maybe I should comment out the tests and leave a note to enable them in 6months/1year when 6.3 is released?

casevh · 2020-11-24T22:22:38Z

I support two different versions of MPFR and then split the tests into separate files. I'll review this once I get 2.1.0 released.

…

On Tue, Nov 24, 2020 at 1:44 PM Seth Troisi ***@***.***> wrote: I inverted the #if which fixes compilation. Tests are still broken because libgmp-dev is version 6.2 Maybe I should comment out the tests and leave a note to enable them in 6months/1year when 6.3 is released? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#284 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMR23Z4COME7BEV7ZFSNYDSRQSMNANCNFSM4T5XB67A> .

sethtroisi · 2022-07-19T20:48:01Z

libgmp could have contained prevprime in 6.2.1, but it didn't.
I fixed the merge conflict. Guess I'll wait another 2 years for a gmp release.

casevh · 2022-07-23T05:30:49Z

I've generally used conditional compilation to only include a new function if the underlying library provides it. But I may have another option for including new features. Don't update your PR just yet.

skirpichev · 2023-09-05T11:01:43Z

@casevh, I think it's time to review this (gmp-6.3.0 is available).

@sethtroisi, could you, please, fix merge conflicts? Documentation structure was changed, let me know if you will need some help.

sethtroisi · 2023-09-05T22:01:49Z

I think I'm testing the correct thing by using

python setup.py build_ext --inplace
pip install .
python test/runtests.py

I have some test failures but they are certainly related to mpc e.g. mpc('nan+nanj') does not match mpc('nannanj').

Let me know if you want any changes.

sethtroisi · 2023-09-05T22:07:20Z

I think CI requires than #425 is merged. Currently GMP 6.3 isn't downloaded which results in PrevPrime returning NotImplemented in the tests.

skirpichev · 2023-09-06T01:20:07Z

src/gmpy2_mpz_misc.c

+static PyObject *
+GMPy_MPZ_Function_PrevPrime(PyObject *self, PyObject *other)
+{
+    #if (__GNU_MP_VERSION > 6) || (__GNU_MP_VERSION == 6 &&  __GNU_MP_VERSION_MINOR >= 3)


Given f8878b6 (and 39a9e48), I would guess, that you should surround the whole function by #if's.

I prefer to only include functions that are supported by the GMP/MPFR/MPC version. I think it causes less confusion than raising NonImpemented. The documentation can include prev_prime with a note "Only present when compiled with GMP 6.3.0 or later."

I believe the documentation is in the build artifacts and I'll check that it contains that note which I added to PyDoc_STRVAR(GMPy_doc_mpz_function_prev_prime.

I believe the documentation is in the build artifacts

No. The documentation build will fail, unless you exclude new function for GMP < 6.3.0.

OR (perhaps, it's better), you can include the prev_prive (a copy of the docstring) to the sphinx docs with the function directive.

I got sphinx running locally and verified that the prev_prime is included (conditional on has_prev_prime which is hardcoded in conf.py as mp_version() >= "GMP 6.3.0")

skirpichev · 2023-09-06T01:24:14Z

test/test_gmpy2_mpz_misc.txt

+Test prev_prime
+---------------
+>>> gmpy2.prev_prime('a')
+Traceback (most recent call last):
+  ...
+TypeError:
+>>> gmpy2.prev_prime(2)
+mpz(2)
+>>> gmpy2.prev_prime(mpz(3))
+mpz(2)
+>>> gmpy2.prev_prime(3)
+mpz(2)
+>>> gmpy2.prev_prime(1000004)
+mpz(1000003)
+>>> gmpy2.prev_prime(1000033)
+mpz(1000003)
+>>> gmpy2.prev_prime(-100)
+mpz(2)
+>>> gmpy2.prev_prime(1)
+mpz(2)
+


Per #377, you shouldn't use test/*.txt files for new tests.

These tests could be moved to the test_mpz.py, I think. And you could use the pytest's skipif fixture to skip them if the GMP library is too old.

Some *.txt are there.

I left one additional test for next_prime(1000000) which I think is a nice test I can remove if you don't want it added here.

skirpichev · 2023-09-06T01:28:49Z

docs/mpz.rst

@@ -105,6 +105,7 @@ mpz Functions
 .. autofunction:: powmod_exp_list
 .. autofunction:: powmod_base_list
 .. autofunction:: powmod_sec
+.. autofunction:: prev_prime


Not sure if we should include this function to the sphinx docs.

But if so, you should do this conditionally, probably with the "only" directive of the sphinx. Also, I think that in this case we should mention in the prev_prime docstring, that this function is available only for certain conditions.

Suggested change

.. autofunction:: prev_prime

.. function:: prev_prime(x, /) -> mpz

Return the previous *probable* prime number < x.

Only present when compiled with GMP 6.3.0 or later.

It took me a few days but I made a conditional tag and got this working with the only directive (thanks for pointing me at it).

The problem with your solution is that this function will not be displayed on the https://gmpy2.readthedocs.io/en/latest/ for a long time. That's why I did this suggestion.

Yep you were perspective in a way I had to learn by making the mistake. Should be fixed now. I tried to run build, tests, and sphinx, with and without gmp 6.3.0.

skirpichev · 2023-09-06T01:47:01Z

src/gmpy2_mpz_misc.c

+                return NULL;
+            }
+            else {
+                if (!mpz_prevprime(result->z, result->z)) {


Docs says in this case the rop is unchanged. Maybe you should return result without any tests?

Or we could raise a ValueError. Silent, undocumented default - looks odd for me.

I changed it to ValueError.

src/gmpy2_mpz_misc.c

skirpichev · 2023-09-06T01:50:59Z

I think I'm testing the correct thing by using

pip install .
python test/runtests.py

Should be enough.

I have some test failures but they are certainly related to mpc e.g. mpc('nan+nanj') does not match mpc('nannanj').

Probably related to #422. If you did a rebase wrt the current master - there should be no failures.

I think CI requires than #425 is merged. Currently GMP 6.3 isn't downloaded which results in PrevPrime returning NotImplemented in the tests.

Yes, #425 is required. But tests should pass in any case.

src/gmpy2.c

codecov-commenter · 2023-09-10T04:43:56Z

Codecov Report

Merging #284 (3277ae3) into master (eb56a3d) will not change coverage.
Report is 9 commits behind head on master.
The diff coverage is 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@           Coverage Diff           @@
##           master     #284   +/-   ##
=======================================
  Coverage   83.63%   83.63%           
=======================================
  Files          49       49           
  Lines       11715    11715           
  Branches     2190     2190           
=======================================
  Hits         9798     9798           
  Misses       1917     1917

Files Changed	Coverage Δ
src/gmpy2.c	`100.00% <ø> (ø)`
src/gmpy2_mpz_misc.c	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

test/test_mpz_functions.txt

Co-authored-by: Sergey B Kirpichev <[email protected]>

casevh · 2023-09-11T00:33:55Z

Thanks.

sethtroisi · 2023-09-11T00:34:28Z

Here I was writing a long question about if anything more was needed. :) Thanks for pulling I'm glad to have gotten this included!

skirpichev · 2023-09-11T00:42:54Z

Here I was writing a long question about if anything more was needed. :)

@sethtroisi, as I said above - you could just use "function" directive without any checks. Else the sphinx docs on the https://gmpy2.readthedocs.io/en/latest/ or in the build artifact will not include the prev_prime() function. @casevh, probably we want to include this unconditionally (with a warning), right?

Amend aleaxit#284

skirpichev · 2023-09-11T01:12:32Z

See #430

sethtroisi force-pushed the master branch 2 times, most recently from e2ac363 to e2f956b Compare July 20, 2022 04:57

Added prev_prime(x)

b24fdea

sethtroisi force-pushed the master branch from e2f956b to b24fdea Compare September 5, 2023 22:04

skirpichev reviewed Sep 6, 2023

View reviewed changes

src/gmpy2_mpz_misc.c Show resolved Hide resolved

sethtroisi added 2 commits September 7, 2023 00:30

Moved prev_prime tests to test/test_mpz.py

97295d6

Added @skipif to test_prev_prime

3b4b027

skirpichev reviewed Sep 7, 2023

View reviewed changes

src/gmpy2.c Show resolved Hide resolved

sethtroisi added 3 commits September 7, 2023 14:39

add conditionals around GMPy_MPZ_Function_PrevPrime and use

7c93d68

import prev_prime conditional on mp_version >= 6.3.0

b7b488c

Make prev_prime documentation conditional only GMP >= 6.3.0

ad460ef

sethtroisi force-pushed the master branch from d020433 to ad460ef Compare September 10, 2023 04:41

skirpichev reviewed Sep 10, 2023

View reviewed changes

test/test_mpz_functions.txt Outdated Show resolved Hide resolved

Update test/test_mpz_functions.txt

3277ae3

Co-authored-by: Sergey B Kirpichev <[email protected]>

casevh merged commit 7f09f58 into aleaxit:master Sep 11, 2023

skirpichev added a commit to skirpichev/gmpy that referenced this pull request Sep 11, 2023

Include prev_prime() to the sphinx docs unconditionaly

a7b1129

Amend aleaxit#284

skirpichev mentioned this pull request Sep 11, 2023

Include prev_prime() to the sphinx docs unconditionaly #430

Merged

sethtroisi deleted the master branch September 11, 2023 04:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added prev_prime(x) #284

Added prev_prime(x) #284

sethtroisi commented Nov 21, 2020 •

edited

Loading

casevh commented Nov 22, 2020

sethtroisi commented Nov 24, 2020

casevh commented Nov 24, 2020 via email

sethtroisi commented Jul 19, 2022 •

edited

Loading

casevh commented Jul 23, 2022

skirpichev commented Sep 5, 2023

sethtroisi commented Sep 5, 2023

sethtroisi commented Sep 5, 2023

skirpichev Sep 6, 2023

casevh Sep 6, 2023

sethtroisi Sep 7, 2023

skirpichev Sep 7, 2023

skirpichev Sep 7, 2023

sethtroisi Sep 10, 2023

skirpichev Sep 6, 2023

sethtroisi Sep 7, 2023

skirpichev Sep 7, 2023

sethtroisi Sep 10, 2023

skirpichev Sep 6, 2023

skirpichev Sep 8, 2023

sethtroisi Sep 10, 2023

skirpichev Sep 10, 2023

sethtroisi Sep 10, 2023 •

edited

Loading

skirpichev Sep 6, 2023 •

edited

Loading

sethtroisi Sep 7, 2023

skirpichev commented Sep 6, 2023

codecov-commenter commented Sep 10, 2023 •

edited

Loading

casevh commented Sep 11, 2023

sethtroisi commented Sep 11, 2023

skirpichev commented Sep 11, 2023

skirpichev commented Sep 11, 2023

-.. autofunction:: prev_prime
+.. function:: prev_prime(x, /) -> mpz
+   Return the previous *probable* prime number < x.
+   Only present when compiled with GMP 6.3.0 or later.

Added prev_prime(x) #284

Added prev_prime(x) #284

Conversation

sethtroisi commented Nov 21, 2020 • edited Loading

casevh commented Nov 22, 2020

sethtroisi commented Nov 24, 2020

casevh commented Nov 24, 2020 via email

sethtroisi commented Jul 19, 2022 • edited Loading

casevh commented Jul 23, 2022

skirpichev commented Sep 5, 2023

sethtroisi commented Sep 5, 2023

sethtroisi commented Sep 5, 2023

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

sethtroisi Sep 10, 2023 • edited Loading

Choose a reason for hiding this comment

skirpichev Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skirpichev commented Sep 6, 2023

codecov-commenter commented Sep 10, 2023 • edited Loading

Codecov Report

casevh commented Sep 11, 2023

sethtroisi commented Sep 11, 2023

skirpichev commented Sep 11, 2023

skirpichev commented Sep 11, 2023

sethtroisi commented Nov 21, 2020 •

edited

Loading

sethtroisi commented Jul 19, 2022 •

edited

Loading

sethtroisi Sep 10, 2023 •

edited

Loading

skirpichev Sep 6, 2023 •

edited

Loading

codecov-commenter commented Sep 10, 2023 •

edited

Loading