Add `radd` etc methods for Cython 3.x #35

oscarbenjamin · 2022-12-14T13:31:07Z

Fixes #34

oscarbenjamin · 2022-12-14T13:49:46Z

src/flint/fmpz.pyx

+    def __rpow__(s, t, m):
+        cdef fmpz_struct tval[1]
+        cdef int stype = FMPZ_UNKNOWN
+        cdef ulong exp
+        u = NotImplemented
+        if m is not None:
+            raise NotImplementedError("modular exponentiation")
+        ttype = fmpz_set_any_ref(tval, t)
+        if ttype != FMPZ_UNKNOWN:
+            u = fmpz.__new__(fmpz)
+            s_ulong = fmpz_get_ui(s.val)
+            fmpz_pow_ui((<fmpz>u).val, tval, s_ulong)


I'm not sure about this.

Actually I see that with current master and Cython 0.29 we have:

>>> import flint >>> flint.fmpz(2) ** flint.fmpz(-2) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "src/flint/fmpz.pyx", line 297, in flint._flint.fmpz.__pow__ fmpz_pow_ui((<fmpz>u).val, sval, c) OverflowError: can't convert negative value to ulong

So I guess the existing __pow__ method should be changed. At least OverflowError is not the right exception to raise.

Python itself gives:

>>> 2 ** -2 0.25 >>> 1 ** -1 1.0 >>> 2 ** 2 4

In principle an fmpq could be returned but I'm not sure if that's better than an error.

gmpy2 returns mpfr:

In [3]: gmpy2.mpz(2) ** -2 Out[3]: mpfr('0.25') In [4]: gmpy2.mpz(2) ** 2 Out[4]: mpz(4)

It's probably good to maintain compatibility with gmpy2.mpz and int itself if possible so that mpz can be used interchangeably with those. I'm not sure what type that really means in the python-flint context though.

fredrik-johansson · 2022-12-14T16:31:50Z

As a general principle, I dislike value-dependent return types. If a larger domain is needed to represent some results of an operation, then either the the return type should always be that of the larger domain, or there should be an exception.

You could support automatic extensions with method options like fmpz(2).pow(-2, extend_domain=True) but of course this will not work with builtin operators. Maybe some kind of context option / context manager for how to handle domain errors?

oscarbenjamin · 2022-12-14T20:14:03Z

If a larger domain is needed to represent some results of an operation, then either the the return type should always be that of the larger domain, or there should be an exception.

So then in this case it should be an exception but probably not OverflowError. Maybe a python_flint specific exception like FlintDomainError would work (it could perhaps also subclass ValueError).

There are two particular cases that could conceivably be handled:

>>> flint.fmpz(1) ** flint.fmpz(-1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "src/flint/fmpz.pyx", line 297, in flint._flint.fmpz.__pow__
    fmpz_pow_ui((<fmpz>u).val, sval, c)
OverflowError: can't convert negative value to ulong
>>> flint.fmpz(-1) ** flint.fmpz(-1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "src/flint/fmpz.pyx", line 297, in flint._flint.fmpz.__pow__
    fmpz_pow_ui((<fmpz>u).val, sval, c)
OverflowError: can't convert negative value to ulong

Both int and gmpy2.mpz still return float in these cases:

>>> 1 ** -1
1.0
>>> (-1) ** -1
-1.0
>>> gmpy2.mpz(1) ** gmpy2.mpz(-1)
mpfr('1.0')
>>> gmpy2.mpz(-1) ** gmpy2.mpz(-1)
mpfr('-1.0')

Maybe it's just not worth handling them for the simplicity of just saying that the exponent should always be nonnegative.

fredrik-johansson · 2022-12-14T23:02:29Z

Sure, there could be a more appropriate exception. Right now error handling is just implemented very lazily in many places by having a conversion from Python type -> C type and letting Cython catch the overflow.

In generic-rings there is much more systematic approach to error handling with two types of exceptions: "domain error" vs "unable to compute". It would be nice to implement something similar in the present python-flint in anticipation of a future merger with that code. But it's also not worth overengineering.

oscarbenjamin · 2023-04-21T10:54:06Z

I've added the __rdunder__ methods to all flint types fmpz, fmpq, nmod, fmpz_poly, ... and also arf, arb and acb.

At this point the test suite passes but there are other types that need to be changed:

acb_mat
acb_poly
acb_series
arb_mat
arb_poly
arb_series

I guess they are not covered as well in the tests.

oscarbenjamin · 2023-04-21T12:41:08Z

Okay, I think that is all types updated now.

oscarbenjamin · 2023-04-21T12:43:51Z

This will now only work with Cython 3.x because I have changed many of the existing __add__ etc methods so that they assume their first argument is of the expected type e.g. acb_mat.__add__ can assume that its first argument s an acb_mat.

It also fails to compile with Cython 3.0.0a12 (because of Cython bugs) but compiles correctly with Cython 3.0.0b2.

This is needed for Cython 3.x compatibility: https://cython.readthedocs.io/en/latest/src/userguide/special_methods.html#arithmetic-methods Similar methods should be added to all python-flint types that have arithmetic operations.

oscarbenjamin · 2023-08-03T11:53:44Z

GitHub Actions CI jobs are failing because downloading the source code for GMP fails. The same command works fine locally:

curl -O https://gmplib.org/download/gmp/gmp-6.2.1.tar.xz

Also I cancelled and restarted it but on the first run the OSX job did successfully download it but the Linux and Windows jobs failed to.

The Cirrus CI job (for OSX arm64) succeeded.

I'm not sure why downloading (mostly) fails from GitHub Actions. Maybe it is just a temporary problem.

fredrik-johansson · 2023-08-03T12:08:58Z

The GMP server is blocking accesses from GitHub because it's effectively being DDoSed: see the notice on https://gmplib.org/

You will need to use a mirror.

oscarbenjamin · 2023-08-03T13:07:46Z

You will need to use a mirror.

Ah, thanks. I've made a GitHub repo to store the GMP release code:

https://github.com/oscarbenjamin/gmp_mirror

The CI scripts should now download from there...

Not sure if there is a better idea than that but it seems reasonable to have all the CI stuff download from GitHub.

oscarbenjamin · 2023-08-03T14:09:49Z

Okay, that worked and now all wheels are built and tests passed.

I've bumped the cython version to 3.0.0 (released two weeks ago) rather than 3.0.0b1.

I've also bumped GMP from 6.2.1 to 6.3.0 and removed the macos_arm64 patch (it shouldn't be needed any more).

If that all passes fine then everything is up to date although open a separate PR to test with flint 3.0.0-alpha1.

oscarbenjamin · 2023-08-03T15:11:38Z

open a separate PR to test with flint 3.0.0-alpha1

I don't seem to be able to build flint 3.0.0 locally on Ubuntu 22.04:

$ ./configure --disable-static
configure: WARNING: unrecognized options: --disable-static
./configure: line 2853: LT_INIT: command not found
checking for a race-free mkdir -p... /usr/bin/mkdir -p
checking for gcc... gcc
checking whether the compiler supports GNU C... no
checking whether gcc accepts -g... no
checking for gcc option to enable C11 features... unsupported
checking for gcc option to enable C99 features... unsupported
checking for gcc option to enable C89 features... unsupported
checking for inline... no
checking for stdio.h... no
checking for stdlib.h... no
checking for string.h... no
checking for inttypes.h... no
checking for stdint.h... no
checking for strings.h... no
checking for sys/stat.h... no
checking for sys/types.h... no
checking for unistd.h... no
checking whether byte ordering is bigendian... no
checking for ldconfig... ldconfig
checking for stdarg.h... no
configure: error: Could not find a mandatory header!

I do have stdarg.h:

$ cat t.c
#include <stdarg.h>
$ gcc -c t.c

Seems to be a problem with the configure script detecting headers somehow.

fredrik-johansson · 2023-08-03T15:15:41Z

This

$ ./configure --disable-static
configure: WARNING: unrecognized options: --disable-static

is weird, as --disable-static certainly should be recognized by flint's configure.

Did you run ./bootstrap.sh and did it report any errors?

oscarbenjamin · 2023-08-03T15:24:16Z

Fresh from the tarball:

$ tar xf flint-3.0.0-alpha1.tar.gz 
$ cd flint-3.0.0-alpha1/
$ ./bootstrap.sh 
autoreconf: export WARNINGS=all
autoreconf: Entering directory '.'
autoreconf: configure.ac: not using Gettext
autoreconf: running: aclocal --force 
autoreconf: configure.ac: tracing
autoreconf: configure.ac: creating directory config
autoreconf: configure.ac: not using Libtool
autoreconf: configure.ac: not using Intltool
autoreconf: configure.ac: not using Gtkdoc
autoreconf: running: /usr/bin/autoconf --force
autoreconf: running: /usr/bin/autoheader --force
autoreconf: configure.ac: not using Automake
autoreconf: linking config/install-sh to /usr/share/autoconf/build-aux/install-sh
autoreconf: Leaving directory '.'
$ ./configure 
./configure: line 2853: LT_INIT: command not found
checking for a race-free mkdir -p... /usr/bin/mkdir -p
checking for gcc... gcc
checking whether the compiler supports GNU C... no
checking whether gcc accepts -g... no
checking for gcc option to enable C11 features... unsupported
checking for gcc option to enable C99 features... unsupported
checking for gcc option to enable C89 features... unsupported
checking for inline... no
checking for stdio.h... no
checking for stdlib.h... no
checking for string.h... no
checking for inttypes.h... no
checking for stdint.h... no
checking for strings.h... no
checking for sys/stat.h... no
checking for sys/types.h... no
checking for unistd.h... no
checking whether byte ordering is bigendian... no
checking for ldconfig... ldconfig
checking for stdarg.h... no
configure: error: Could not find a mandatory header!

In config.log it seems to think that everything fails to compile regardless of the exit code (if I'm interpreting it correctly):

configure:4858: result: no
configure:4858: checking for stdlib.h
configure:4858: gcc -c   conftest.c >&5
configure:4858: $? = 0
configure: failed program was:

fredrik-johansson · 2023-08-03T15:28:40Z

Judging from

./configure: line 3638: LT_INIT: command not found

perhaps libtool is missing on your system?

oscarbenjamin · 2023-08-03T15:32:08Z

Thanks, that fixed it:

sudo apt install libtool-bin

Now:

$ ./bootstrap.sh 
autoreconf: export WARNINGS=all
autoreconf: Entering directory '.'
autoreconf: configure.ac: not using Gettext
autoreconf: running: aclocal --force 
autoreconf: configure.ac: tracing
autoreconf: running: libtoolize --force
libtoolize: putting auxiliary files in AC_CONFIG_AUX_DIR, 'config'.
libtoolize: linking file 'config/ltmain.sh'
libtoolize: Consider adding 'AC_CONFIG_MACRO_DIRS([m4])' to configure.ac,
libtoolize: and rerunning libtoolize and aclocal.
libtoolize: Consider adding '-I m4' to ACLOCAL_AMFLAGS in Makefile.am.
autoreconf: configure.ac: not using Intltool
autoreconf: configure.ac: not using Gtkdoc
autoreconf: running: aclocal --force 
autoreconf: running: /usr/bin/autoconf --force
autoreconf: running: /usr/bin/autoheader --force
autoreconf: configure.ac: not using Automake
autoreconf: linking config/config.sub to /usr/share/autoconf/build-aux/config.sub
autoreconf: linking config/config.guess to /usr/share/autoconf/build-aux/config.guess
autoreconf: Leaving directory '.'
$ ./configure --disable-static
checking build system type... x86_64-pc-linux-gnu
checking host system type... x86_64-pc-linux-gnu
checking how to print strings... printf
checking for gcc... gcc
checking whether the C compiler works... yes
checking for C compiler default output file name... a.out
checking for suffix of executables... 
checking whether we are cross compiling... no
checking for suffix of object files... o
checking whether the compiler supports GNU C... yes
checking whether gcc accepts -g... yes
checking for gcc option to enable C11 features... none needed
checking for a sed that does not truncate output... /usr/bin/sed
checking for grep that handles long lines and -e... /usr/bin/grep
checking for egrep... /usr/bin/grep -E
checking for fgrep... /usr/bin/grep -F
checking for ld used by gcc... /usr/bin/ld
checking if the linker (/usr/bin/ld) is GNU ld... yes
checking for BSD- or MS-compatible name lister (nm)... /usr/bin/nm -B
checking the name lister (/usr/bin/nm -B) interface... BSD nm
checking whether ln -s works... yes
checking the maximum length of command line arguments... 1572864
checking how to convert x86_64-pc-linux-gnu file names to x86_64-pc-linux-gnu format... func_convert_file_noop
checking how to convert x86_64-pc-linux-gnu file names to toolchain format... func_convert_file_noop
checking for /usr/bin/ld option to reload object files... -r
checking for objdump... objdump
checking how to recognize dependent libraries... pass_all
checking for dlltool... no
checking how to associate runtime and link libraries... printf %s\n
checking for ar... ar
checking for archiver @FILE support... @
checking for strip... strip
checking for ranlib... ranlib
checking for gawk... no
checking for mawk... mawk
checking command to parse /usr/bin/nm -B output from gcc object... ok
checking for sysroot... no
checking for a working dd... /usr/bin/dd
checking how to truncate binary pipes... /usr/bin/dd bs=4096 count=1
checking for mt... mt
checking if mt is a manifest tool... no
checking for stdio.h... yes
checking for stdlib.h... yes
checking for string.h... yes
checking for inttypes.h... yes
checking for stdint.h... yes
checking for strings.h... yes
checking for sys/stat.h... yes
checking for sys/types.h... yes
checking for unistd.h... yes
checking for dlfcn.h... yes
checking for objdir... .libs
checking if gcc supports -fno-rtti -fno-exceptions... no
checking for gcc option to produce PIC... -fPIC -DPIC
checking if gcc PIC flag -fPIC -DPIC works... yes
checking if gcc static flag -static works... yes
checking if gcc supports -c -o file.o... yes
checking if gcc supports -c -o file.o... (cached) yes
checking whether the gcc linker (/usr/bin/ld -m elf_x86_64) supports shared libraries... yes
checking whether -lc should be explicitly linked in... no
checking dynamic linker characteristics... GNU/Linux ld.so
checking how to hardcode library paths into programs... immediate
checking whether stripping libraries is possible... yes
checking if libtool supports shared libraries... yes
checking whether to build shared libraries... yes
checking whether to build static libraries... no
checking for a race-free mkdir -p... /usr/bin/mkdir -p
checking for inline... inline
checking whether byte ordering is bigendian... no
checking for ldconfig... ldconfig
checking for stdarg.h... yes
checking for math.h... yes
checking for float.h... yes
checking for errno.h... yes
checking for gmp.h... yes
checking for mpfr.h... yes
checking for fenv.h... yes
checking for alloca.h... yes
checking for malloc.h... yes
checking for sys/param.h... yes
checking for windows.h... no
checking for arm_neon.h... no
checking for x86intrin.h... yes
checking for immintrin.h... yes
checking for pthread.h... yes
checking for fabs in -lm... yes
checking for __gmpz_init in -lgmp... yes
checking for __gmpn_gcd_11 in -lgmp... yes
checking for __gmpn_div_q in -lgmp... yes
checking for mpfr_init in -lmpfr... yes
checking whether gcc accepts -pthread... yes
checking if gcc supports TLS handle... __thread
checking if cpu_set_t is supported... yes
checking if alloca works... yes
checking if gcc has popcount intrinsics... yes
checking if gcc has CLZ intrinsics... yes
checking if gcc has CTZ intrinsics... yes
checking if gcc has __builtin_constant_p... yes
checking whether gcc accepts -Werror=unknown-warning-option... no
checking whether gcc accepts -Wall... yes
checking whether gcc accepts -Wno-stringop-overread... yes
checking whether gcc accepts -Wno-stringop-overflow... yes
checking whether gcc accepts -Werror=implicit-function-declaration... yes
checking whether gcc accepts -O2... yes
checking whether gcc accepts -std=c11... yes
checking whether gcc accepts -pedantic... yes
checking whether gcc accepts -funroll-loops... yes
checking whether fft_small module is available... no
configure: creating ./config.status
config.status: creating src/fft_tuning.h
config.status: creating src/fmpz/fmpz.c
config.status: creating Makefile
config.status: creating flint.pc
config.status: creating src/flint-config.h
config.status: executing libtool commands

oscarbenjamin · 2023-08-03T16:09:52Z

Okay, looks like getting python_flint to work with flint 3.0.0-alpha1 is just a simple case of renaming the arb headers and removing -larb in setup.py. I have a working build with all tests passing. I'll open a separate PR with that so to test across all CI platforms and have something ready for when flint 3 is released.

To summarise the changes in this PR:

Most of the diff is just adding methods like __radd__ that are needed in Cython 3 but were not needed in previous versions of Cython.
The changes to the __add__ etc methods means that this is no longer compatible with Cython<3 so the build requirements are now Cython>=3.0.0.
The version of GMP is updated to 6.3.0 and the macos arm64 patch is no longer applied.
The version of Cython used in CI is now 3.0.0.

It could be possible to make this work with all Cython versions by using a configuration option instead of changing the code but I think that it is better to update the code to the latest default Cython mode and given that Cython is a build-time dependency that can be pinned there is no real reason to support multiple versions.

Actually I can remove the version pin on Cython in CI here since the default current version works. I'll do that now.

Do you want to check the PR at all or am I good to merge it?

oscarbenjamin · 2023-08-03T16:19:48Z

To summarise the changes in this PR:

Oh, the other change is that CI no longer downloads from gmplib.org which currently causes CI to fail.

oscarbenjamin · 2023-08-03T18:27:41Z

Okay, let's get this one in.

oscarbenjamin mentioned this pull request Dec 14, 2022

wip: add coverage testing #29

Merged

oscarbenjamin commented Dec 14, 2022

View reviewed changes

oscarbenjamin force-pushed the pr_cython3 branch from 3aa4a76 to f759cc5 Compare December 14, 2022 13:54

oscarbenjamin mentioned this pull request Dec 14, 2022

maint: move cython language_level to 3 #36

Merged

oscarbenjamin closed this Apr 20, 2023

oscarbenjamin reopened this Apr 20, 2023

oscarbenjamin force-pushed the pr_cython3 branch from 59b9f35 to 070b7e5 Compare April 21, 2023 10:48

oscarbenjamin mentioned this pull request Apr 21, 2023

Compatibility with cython 3 #34

Closed

oscarbenjamin added 16 commits August 3, 2023 12:02

test: use cython==3.0.0a11 to build in CI

b24ec90

fix: add __radd__ etc to fmpz

5fe55f3

This is needed for Cython 3.x compatibility: https://cython.readthedocs.io/en/latest/src/userguide/special_methods.html#arithmetic-methods Similar methods should be added to all python-flint types that have arithmetic operations.

test: also use cython 3 on Windows

12885b6

fix: remove redundant paths in dunder methods

3bacb07

fix: add __rdunder__ methods for fmpq and nmod

978696a

fix: add __rdunder__ fmpz_mat, fmpq_mat and nmp_mat

1556939

fix: add __rdunder__ fmpz_poly, fmpq_poly and nmp_poly

19d7526

fix: add __rdunder__ methods fmpz_series, fmpq_series

4d9175b

fix: add __rdunder__ methods for arb and acb

3c03077

Use cython 3.0.0b2

fcda89f

fix: add modulus arg to acb.__rpow__

5082c78

fix: __rdunder__ acb_poly, acb_series, arb_poly, arb_series

5a4a2aa

Use cython==3.0.0b2 in Cirruc CI as well

d3c2519

fix: add __rdunder__ methods for acb_mat and arb_mat

0a8bf5c

Add redundant type casts for older Cython

913f69c

fix: fmpz_mat * nmod_mat -> nmod_mat

a390bdc

oscarbenjamin force-pushed the pr_cython3 branch from d9ca14e to a390bdc Compare August 3, 2023 11:33

Use GMP mirror from GitHub in CI

12bc215

oscarbenjamin added 2 commits August 3, 2023 15:01

Update Cython -> 3.0.0, GMP -> 6.3.0

e1c0c70

Do not apply GMP macos_arm64 patch

d4db446

oscarbenjamin mentioned this pull request Aug 3, 2023

Bump flint to version 3 #43

Closed

Remove version constraints for Cython

0370688

oscarbenjamin merged commit 027dd1f into flintlib:master Aug 3, 2023
22 checks passed

oscarbenjamin deleted the pr_cython3 branch August 3, 2023 18:27

oscarbenjamin mentioned this pull request Aug 27, 2024

[BUG] Assertion error from encode_single_position, cython/cython#6366

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `radd` etc methods for Cython 3.x #35

Add `radd` etc methods for Cython 3.x #35

oscarbenjamin commented Dec 14, 2022 •

edited

Loading

oscarbenjamin Dec 14, 2022

oscarbenjamin Dec 14, 2022

fredrik-johansson commented Dec 14, 2022

oscarbenjamin commented Dec 14, 2022

fredrik-johansson commented Dec 14, 2022

oscarbenjamin commented Apr 21, 2023

oscarbenjamin commented Apr 21, 2023

oscarbenjamin commented Apr 21, 2023

oscarbenjamin commented Aug 3, 2023

fredrik-johansson commented Aug 3, 2023

oscarbenjamin commented Aug 3, 2023

oscarbenjamin commented Aug 3, 2023

oscarbenjamin commented Aug 3, 2023

fredrik-johansson commented Aug 3, 2023

oscarbenjamin commented Aug 3, 2023

fredrik-johansson commented Aug 3, 2023

oscarbenjamin commented Aug 3, 2023

oscarbenjamin commented Aug 3, 2023

oscarbenjamin commented Aug 3, 2023

oscarbenjamin commented Aug 3, 2023

Add __radd__ etc methods for Cython 3.x #35

Add __radd__ etc methods for Cython 3.x #35

Conversation

oscarbenjamin commented Dec 14, 2022 • edited Loading

oscarbenjamin Dec 14, 2022

Choose a reason for hiding this comment

oscarbenjamin Dec 14, 2022

Choose a reason for hiding this comment

fredrik-johansson commented Dec 14, 2022

oscarbenjamin commented Dec 14, 2022

fredrik-johansson commented Dec 14, 2022

oscarbenjamin commented Apr 21, 2023

oscarbenjamin commented Apr 21, 2023

oscarbenjamin commented Apr 21, 2023

oscarbenjamin commented Aug 3, 2023

fredrik-johansson commented Aug 3, 2023

oscarbenjamin commented Aug 3, 2023

oscarbenjamin commented Aug 3, 2023

oscarbenjamin commented Aug 3, 2023

fredrik-johansson commented Aug 3, 2023

oscarbenjamin commented Aug 3, 2023

fredrik-johansson commented Aug 3, 2023

oscarbenjamin commented Aug 3, 2023

oscarbenjamin commented Aug 3, 2023

oscarbenjamin commented Aug 3, 2023

oscarbenjamin commented Aug 3, 2023

Add `radd` etc methods for Cython 3.x #35

Add `radd` etc methods for Cython 3.x #35

oscarbenjamin commented Dec 14, 2022 •

edited

Loading