Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
oscarbenjamin opened this issue Aug 14, 2023 · 7 comments
Open

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

oscarbenjamin opened this issue Aug 14, 2023 · 7 comments

Comments

@oscarbenjamin
Copy link
Collaborator

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:

cdef inline int fmpz_set_pylong(fmpz_t x, obj):
cdef int overflow
cdef slong longval
longval = pylong_as_slong(<PyObject*>obj, &overflow)
if overflow:
s = "%x" % obj
fmpz_set_str(x, chars_from_str(s), 16)
else:
fmpz_set_si(x, longval)
cdef inline int fmpz_set_python(fmpz_t x, obj):
if PY_MAJOR_VERSION < 3 and PyInt_Check(<PyObject*>obj):
fmpz_set_si(x, PyInt_AS_LONG(<PyObject*>obj))
return 1
if PyLong_Check(<PyObject*>obj):
fmpz_set_pylong(x, obj)
return 1
return 0
cdef fmpz_get_intlong(fmpz_t x):
"""
Convert fmpz_t to a Python int or long.
"""
cdef char * s
if COEFF_IS_MPZ(x[0]):
s = fmpz_get_str(NULL, 16, x)
v = int(str_from_chars(s), 16)
libc.stdlib.free(s)
return v
else:
return <slong>x[0]

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
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Contributor

deinst commented Aug 18, 2023

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
Copy link
Collaborator Author

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
Copy link

casevh commented Oct 4, 2023

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
Copy link
Contributor

deinst commented Oct 4, 2023

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
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants