Skip to content

Conversation

@oscarbenjamin
Copy link
Collaborator

This is a companion to the SymPy PR adding flint for ground types:

sympy/sympy#25474

Various things are added:

  • numerator/denominator for fmpz and fmpq.
  • __int__ and __floor__ methods.
  • bitwise operators & etc for fmpz
  • modular exponentiation pow(a, b, c) for fmpz.
  • fmpq ** fmpz.

Comment on lines 402 to 408
def __lshift__(self, other):
if typecheck(other, fmpz):
other = int(other)
if typecheck(other, int):
u = fmpz.__new__(fmpz)
fmpz_mul_2exp((<fmpz>u).val, self.val, other)
return u
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably need to check if other is negative.

test/test.py Outdated
Comment on lines 91 to 104
assert ltype(s) & rtype(t) == s & t
assert ~ltype(s) == ~s

# XXX: Some sort of bug here means that the following fails
# for values of s and t bigger than the word size.
if abs(s) < 2**62 and abs(t) < 2**62:
assert ltype(s) | rtype(t) == s | t
else:
# This still works so somehow the internal representation
# of the fmpz made by fmpz_or is such that fmpz_equal
# returns false even though the values look the same.
assert str(ltype(s) | rtype(t)) == str(s | t)

assert ltype(s) ^ rtype(t) == s ^ t
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what is happening here but I think that the __or__ method here is correct so maybe this is a Flint bug. The __and__ and __xor__ methods have the same code and tests and they work fine.

It seems that somehow fmpz_or creates an fmpz that fmpz_equal considers different even though its string representation is the same:

>>> flint.fmpz(2**62) | -1
-1
>>> flint.fmpz(2**62) | -1 == -1
False
>>> flint.fmpz(2**62-1) | -1
-1
>>> flint.fmpz(2**62-1) | -1 == -1
True

I'm not sure how to debug this via python-flint.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is a bug in fmpz_or. Fixed now in flintlib/flint@9809014.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speedy work!

@oscarbenjamin
Copy link
Collaborator Author

Okay, this PR is complete now and represents all of the changes needed for python-flint to be usable by SymPy as the ground types plus a few more for completeness.

It is basically just adding missing operators/methods that are usually supported by implementations of Z or Q so that fmpz and fmpq can work better as a drop-in replacement for int/Fraction or gmpy2's mpz/mpq.

There are still some differences e.g.:

In [1]: import gmpy2

In [2]: import flint

In [3]: flint.fmpz(3) / flint.fmpz(2)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[3], line 1
----> 1 flint.fmpz(3) / flint.fmpz(2)

TypeError: unsupported operand type(s) for /: 'flint._flint.fmpz' and 'flint._flint.fmpz'

In [4]: gmpy2.mpz(3) / gmpy2.mpz(2)
Out[4]: mpfr('1.5')

In [5]: gmpy2.mpz(3.0)
Out[5]: mpz(3)

In [6]: flint.fmpz(3.0)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[6], line 1
----> 1 flint.fmpz(3.0)

File src/flint/fmpz.pyx:92, in flint._flint.fmpz.__init__()

TypeError: cannot create fmpz from type <class 'float'>

These lead to a couple of doctest failures in SymPy but I think that it is something that can be handled on SymPy's side. The doctest for division is just a doctest saying not to use / with ZZ:
https://github.com/sympy/sympy/blob/b68285262a06a914d9d920aba980d726be2efc39/sympy/polys/domains/domain.py#L1101-L1114
The other failure is for float conversion:
https://github.com/sympy/sympy/blob/b68285262a06a914d9d920aba980d726be2efc39/sympy/polys/densebasic.py#L477-L491
I don't fully understand what that is but I think it is something that can be changed on the SymPy side.

I will add a temporary fix for the fmpz_or bug but then I'll merge this if CI passes and then this is at the point where I would like to put out a release of python-flint so it can be tested in SymPy CI and by other SymPy contributors.

@oscarbenjamin
Copy link
Collaborator Author

Okay, this looks good. I'll merge this and open a new PR to prepare for release.

@oscarbenjamin oscarbenjamin merged commit deb4448 into flintlib:master Aug 8, 2023
@oscarbenjamin oscarbenjamin deleted the pr_sympy_integration branch August 8, 2023 12:26
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

Successfully merging this pull request may close these issues.

2 participants