Repository navigation
Add __radd__ etc methods for Cython 3.x - #35
oscarbenjamin merged 20 commits into
Conversation
Sorry, something went wrong.
| 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) |
There was a problem hiding this comment.
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
4In principle an fmpq could be returned but I'm not sure if that's better than an error.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
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 ulongBoth 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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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:
I guess they are not covered as well in the tests. |
Sorry, something went wrong.
|
Okay, I think that is all types updated now. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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.cSeems to be a problem with the configure script detecting headers somehow. |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
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: |
Sorry, something went wrong.
|
Judging from ./configure: line 3638: LT_INIT: command not found perhaps libtool is missing on your system? |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
|
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:
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? |
Sorry, something went wrong.
Oh, the other change is that CI no longer downloads from gmplib.org which currently causes CI to fail. |
Sorry, something went wrong.
|
Okay, let's get this one in. |
Sorry, something went wrong.
Fixes #34