Added prev_prime(x) - Githubissues

sethtroisi commented 3 years ago

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 commented 3 years ago

Hi Seth,

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

sethtroisi commented 3 years ago

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 commented 3 years ago

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 notifications@github.com 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 https://github.com/aleaxit/gmpy/pull/284#issuecomment-733250787, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMR23Z4COME7BEV7ZFSNYDSRQSMNANCNFSM4T5XB67A .

sethtroisi commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 https://github.com/aleaxit/gmpy/pull/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.

codecov-commenter commented 1 year ago

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%.

:exclamation: 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

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

casevh commented 1 year ago

Thanks.

sethtroisi commented 1 year ago

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 commented 1 year ago

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?

skirpichev commented 1 year ago

See #430

aleaxit / gmpy

Added prev_prime(x) #284

Codecov Report