flintlib / python-flint

Python bindings for Flint and Arb
MIT License
132 stars 27 forks source link

Conversions to/from other types like int gmpy.mpz etc. #56

Open oscarbenjamin opened 1 year ago

oscarbenjamin commented 1 year ago

Currently it is a bit awkward to use flint's fmpz and fmpq because they do not interoperate with anything else:

In [1]: import flint

In [2]: import gmpy2

In [3]: flint.fmpz(2) * gmpy2.mpz(2)
...
TypeError: unsupported operand type(s) for *: 'flint._flint.fmpz' and 'mpz'

I'm not sure that coercion in binary operations really makes sense because e.g. in the above case it is not clear whether the result should be returned as fmpz or mpz. However at least explicit conversions would be nice:

In [4]: flint.fmpz(gmpy2.mpz(2))
...
TypeError: cannot create fmpz from type <class 'mpz'>

In [5]: gmpy2.mpz(flint.fmpz(2))
Out[5]: mpz(2)

The gmpy2 conversion above works (I think) because it uses __index__ which is defined for fmpz. A basic improvement would be for fmpz.__new__ to use __index__ when converting non-int types and then conversion from mpz would at least work.

Using __index__ means going via CPython's int which is inefficient because there is a double conversion and a different limb size. It would be nice if there was a more direct way to convert between mpz and fmpz but I am not sure how easy that is to do if there is any possibility that limb sizes might be set differently at build time.

Converting to from Python int is slow. Currently the conversion goes via a hex Python string: https://github.com/flintlib/python-flint/blob/99fa557c8c358cfeec307c6ed11c1315893a9ca9/src/flint/fmpz.pyx#L1-L31 Ideally there would be some code that could convert more directly between the two different representations with their different limb sizes although I am not sure if CPython provides any "public" API that could be used for this.

Conversion to float works for fmpz but not fmpq:

In [8]: float(flint.fmpz(2))
Out[8]: 2.0

In [9]: float(flint.fmpq(2))
---------------------------------------------------------------------------
TypeError

This could be fixed by defining fmpq.__float__.

Converting from float does not work:

In [11]: flint.fmpz(2.0)
---------------------------------------------------------------------------
TypeError 

In [12]: flint.fmpq(2.0)
---------------------------------------------------------------------------
ValueError

I think that it would be nice to enable this for fmpq but I am not sure about fmpz.

Possibly the stdlib fractions module should be able to coerce:

In [13]: import fractions

In [14]: fractions.Fraction(1, 2) * flint.fmpz(2)
---------------------------------------------------------------------------
TypeError

At least I think that the numeric tower defines numerator and denominator so this could be made to work by checking those:

In [16]: q = fractions.Fraction(2, 3)

In [17]: flint.fmpq(q) # the error message could be improved
...
ValueError: cannot create fmpq from object of type <class 'NotImplementedType'>

In [18]: flint.fmpq(q.numerator, q.denominator)
Out[18]: 2/3

Interaction with mpmath also does not work. I wonder though if it makes more sense to support that on the mpmath side:

In [19]: import mpmath

In [21]: mpmath.mpf(2) * flint.fmpz(2)
---------------------------------------------------------------------------
TypeError 

Perhaps mpmath should have the option to use python-flint for ground types and should add support for coercing python-flint types where it makes sense.

GiacomoPope commented 1 year ago

Small comment, but for flint.fmpz(2.0) you could mirror how python behaves by first casting the inner float to an int before continuing, this has an additional cost, but would cover the case where a user accidentally asks for flint.fmpz(2/4) rather than flint.fmpz(2//4).

oscarbenjamin commented 1 year ago

the case where a user accidentally asks for flint.fmpz(2/4) rather than flint.fmpz(2//4).

This example demonstrates the problem with allowing either flint.fmpz(2.0) or flint.fmpq(2.0). The first question is whether to allow these at all given that they might silently do the wrong thing if used incorrectly.

If flint.fmpz(2/3) uses floor rounding (like int and gmpy2.mpz) then a small rounding error might get turned into a bigger error e.g.:

In [5]: [int(7**n/7) * 7 == 7**n for n in range(10, 30)]
Out[5]: 
[True,
 True,
 True,
 True,
 True,
 True,
 True,
 True,
 True,
 True,
 False,
 False,
 False,
 False,
 False,
 False,
 False,
 False,
 False,
 False]

Likewise if fmpq(2.0) is allowed then we run the risk of other bugs like someone thinking that you can do fmpq(1/3):

In [6]: from fractions import Fraction

In [7]: Fraction(1/3)
Out[7]: Fraction(6004799503160661, 18014398509481984)

I don't know what behaviour is best here. Currently in downstream code I effectively have things like fmpz(int(num)) just because ultimately I need that conversion and also I need the code to be compatible with gmpy2 and Python's int. Probably I would prefer it if gmpy2 and int provided better ways to do that conversion but ultimately I need these types to be compatible somehow if I am going to use them interchangeably.

If we don't want to allow these constrcutor conversions by default what should be provided is some way to allow them explicitly though and for fmpz there should be a way to control rounding when it is needed.

deinst commented 1 year ago

It seems to me that the simplest solution to this would be to just depend on gmpy2. The conversions from mpz to fmpz and mpq to fmpq in flint are almost trivial. gmpy2 has implemented efficient conversions from python ints to mpz and vice-versa and has float <--> mpq conversions implemented efficiently as well.

The second simplest solution would be to copy the gmpy2 C conversion code into cython. The problem with this is that gmpy2 seems to depend on some undocumented internals of the Python integers, and I am not sure we want to do that.

oscarbenjamin commented 1 year ago

Depending on gmpy2 would be nice if we can figure out a way to share a build of GMP and MPFR with gmpy2. I'm just not sure how to share C-level dependencies in different Python packages. Generally Python packaging does not seem to support that case well. We would need gmpy2 to expose some sort of interface that allows finding the headers at build time and also the shared library at runtime. Probably #52 is a good place to discuss that since the gmpy2 maintainer already suggested something similar there.

casevh commented 1 year ago

Quick comment on conversions.

When gmpy2 wants to convert another object to a gmpy2 type, it checks the other object for special methods: __mpz__, __mpq__, __mpfr__, or __mpc__. It is the responsibility of the other object to know how to convert the gmpy2 type. This has the side-effect that flint.fmpz(2) + gmpy2.mpz(3) will return gmpy2.mpz(5). This may or may not be what you want.

gmpy2 does expose a C-API that allows another extension module to import gmpy2 and . That automatically loads GMP, MPFR, and MPC dlls. See the test_cython directory and gmpy2.pxd for examples.

deinst commented 1 year ago

Is there a way of getting the gmp and mpfr library and include file directories? If we could get those we could build flint to use the same underlying libraries,

I do think that we probably need to implement similar methods, at least __acb__, __arb__, __fmpq__, __fmpz__. Right now we have increasingly complicated functions for seeing if a type converts to an fmpq, for example.

I am less convinced that we need functions like __fmpz_mpoly__, etc. , though I am not convinced that we don't.

oscarbenjamin commented 1 year ago

If we could get those we could build flint to use the same underlying libraries,

It is tricky to do this when distributing wheels.

If you build everything locally then obviously you build those libraries once locally and build both gmpy2 and python-flint to use the same libraries. Same goes for how conda would do it. With wheels in PyPI though this would create an ABI dependency between the wheels and the general PyPI packaging ecosystem has no way to understand ABI dependencies like this.

For example if someone installed gmpy2 not from PyPI and then did pip install python-flint then the python-flint wheel might not be ABI compatible with the installed gmpy2 binaries even if the gmpy2 release version matches python-flint's version constraints. Making this work needs a distinction between ABI version constraints that apply to the particular binary wheels rather than API version constraints that apply to the versions of the source code when building from source. Python's general package metadata has no way to represent this though.