Repository navigation
Adding Fmpz mpoly #59, cont. - #132
oscarbenjamin merged 106 commits into
Conversation
Sorry, something went wrong.
Split nmod_poly_factor out from nmod_poly Remove arb from setup.py
Added a skeleton pyproject.toml added a simple setuptools setup.py THere is no good replacement to use instead of numpy.distutils to get the default library and include paths. Any suggestions would be welcome.
The arb lib is now part of flint so all paths are prefixed with flint/
added missing fmpz_mpoly_sort_terms to _flint.pxd updated fmpz_mpoly_context to include names and other term orders Fixed up arithmetic to use python 3 protocol and removed automatic conversion.
Made fmpz_mpoly_ctx return a string instead of bytes.
I hope these work for CI. I currently don't have flint 3.0 or whatever the matching arb lib is.
Some slight adjustments to flint_mpoly_context and fmpz_mpoly needed. It now works on my machine.
Also added tests, and a few __iop__ methods to fmpz_mpoly
Yes, this does not compile. I need to go off on another branch to experiment with solutions.
Added infrastructure for fmpq_mpoly fixed up new import system
Forgot to add it to last commit
Also respliced fmpz_mpoly and fmpq_mpoly back into doctests.
Added submodule to setup Added rational function class to flint_base Added interface to flintlib Added skeleton fmpz_mpoly_q to types
Added missing fmpz_clear for exponents in creating fmpz_mpoly from dictionary Fixed spacing in converting fmpq_mpoly to string
also basic ops on fmpz_mpoly_q
Also added method to get context from a fmpq_mpoly so the tests could check that it was correct.
|
I think that this is probably good to merge now. I will go through the full diff shortly. Before we put out this out in a final release it would be good to have at least an open PR on the sympy side that makes use of this internally as part of either DMP or PolyElement: I have previously written such wrappers like DUP_Flint for fmpz_poly and fmpq_poly and _DFM for fmpz_mat, fmpq_mat, nmod_mat and fmpz_mod_mat as well as making use of fmpz, fmpq and nmod and fmpz_mod directly. In each case by making a pull request to integrate this into sympy I found lots of problems like:
Each time I ended up having at least one followup PR to python-flint before it was ready for the sympy PR to be merged. The sympy test suite is much more extensive than the python-flint one and exercises many more corners of the interface. |
Sorry, something went wrong.
|
Wonderful to hear. I've always thought of a new DMP_Flint class being added, similar to the existing DUP_Flint. I'm unsure how integration on the PolyElement side of things would work. I haven't dived into that section of the code base yet.
I'm sure we will as well. |
Sorry, something went wrong.
|
You might find it helpful to read this: |
Sorry, something went wrong.
oscarbenjamin
left a comment
There was a problem hiding this comment.
This is definitely close but there are a few things I picked up on while working through.
Sorry, something went wrong.
| def keys(self): | ||
| return self.monoms() | ||
|
|
||
| def values(self): | ||
| return self.coeffs() | ||
|
|
||
| def items(self): | ||
| """ | ||
| Return the exponent vectors and coefficient of each term. | ||
|
|
||
| >>> from flint import fmpq_mpoly_ctx, Ordering | ||
| >>> ctx = fmpq_mpoly_ctx.get_context(2, Ordering.lex, 'x') | ||
| >>> f = ctx.from_dict({(0, 0): 1, (1, 0): 2, (0, 1): 3, (1, 1): 4}) | ||
| >>> list(f.items()) | ||
| [((1, 1), 4), ((1, 0), 2), ((0, 1), 3), ((0, 0), 1)] | ||
|
|
||
| """ | ||
| return zip(self.keys(), self.values()) |
There was a problem hiding this comment.
Maybe these should just be removed now.
Sorry, something went wrong.
There was a problem hiding this comment.
Now that we have monoms and coeffs which perform the same role.
Sorry, something went wrong.
There was a problem hiding this comment.
Agreed, but I will note here that .items is slightly different that the .terms currently implemented (and sympy), it returns a list of the terms of the polynomials rather than a tuple of exponent tuple and coefficient. (I swear I commented about that but I cannot find it)
I'm tempting to move the current .terms behaviour into another function or remove it, making .terms align with sympy. Though I'm not sure what I would name it
Sorry, something went wrong.
| fmpq_mpoly_get_term_coeff_fmpq(v.val, self.val, i, self.ctx.val) | ||
| return v | ||
|
|
||
| def exponent_tuple(self, slong i): |
There was a problem hiding this comment.
Given that there is monoms perhaps the name here should be similar:
In [45]: [p.exponent_tuple(i) for i in range(2)] == p.monoms()
Out[45]: True
Sorry, something went wrong.
There was a problem hiding this comment.
I've renamed exponent_tuple to monomial, inline with the coefficient function.
It may be my inexperience with SymPy and python-flint but from my perspective as a user I would expect the mathematically named functions to not return python representations of those things. For example
- monomial returns a tuple of exponent values, not a monomial in the form of a fmp[zq]_mpoly.
- monoms returns a list of tuples of exponent values, not a sequence of monomials in the form of fmp[zq]_mpolys.
- terms returns a zip of coefficients and tuples of exponent values, not a sequence of terms in the form of fmp[zq]_mpolys.
Given that the flint interface provides easy methods for this I think that they would be reasonable functions to have do so
Sorry, something went wrong.
| def __imul__(self, other): | ||
| if typecheck(other, fmpz_mpoly): | ||
| if (<fmpz_mpoly>self).ctx is not (<fmpz_mpoly>other).ctx: | ||
| raise IncompatibleContextError(f"{(<fmpz_mpoly>self).ctx} is not {(<fmpz_mpoly>other).ctx}") | ||
| fmpz_mpoly_mul((<fmpz_mpoly>self).val, (<fmpz_mpoly>self).val, (<fmpz_mpoly>other).val, self.ctx.val) | ||
| return self |
There was a problem hiding this comment.
This is different from how univariate polynomials work in python-flint:
In [85]: p = fmpz_poly([1, 2, 3])
In [86]: p
Out[86]: 3*x^2 + 2*x + 1
In [87]: p2 = p
In [88]: p *= p
In [89]: p
Out[89]: 9*x^4 + 12*x^3 + 10*x^2 + 4*x + 1
In [90]: p2
Out[90]: 3*x^2 + 2*x + 1
In [91]: ctx = fmpq_mpoly_ctx(2, Ordering.lex, ['x','y'])
In [93]: p3 = fmpq_mpoly('2*x^2 + 2*x + 1', ctx)
In [94]: p4 = p3
In [95]: p3 *= p3
In [96]: p3
Out[96]: 4*x^4 + 8*x^3 + 8*x^2 + 4*x + 1
In [97]: p4
Out[97]: 4*x^4 + 8*x^3 + 8*x^2 + 4*x + 1
There is an unresolved tension here about whether polynomials should behave like mutable or immutable objects. I imagine that having p3 *= p3 cause an in-place mutation will trip someone up somewhere who doesn't realise that copies need to be made. Certainly in the context of SymPy the expectation is that such objects can be treated as immutable and therefore do not need to be copied.
Sorry, something went wrong.
There was a problem hiding this comment.
I can imagine that having *= perform an in-place mutation would mess something up somewhere when it is not expected.
This is a bit different from other more explicit mutations like p[k] = v.
Sorry, something went wrong.
There was a problem hiding this comment.
This is a bit different from other more explicit mutations like p[k] = v.
I was going to comment about that. Immutability can be a pretty nice property but flint does support mutation and I think when it's an explicit "flint based" mutation it should be kept but the "mutations" imposed by Python operands should be avoided.
Sorry, something went wrong.
There was a problem hiding this comment.
I agree it would be nice to expose mutability in some way but overloading the __iadd__ and __imul__ operators will break generic Python code.
Sorry, something went wrong.
There was a problem hiding this comment.
Perhaps there could be explicit methods like p1.imul(p2) for in-place mutation.
There could also be a p.hash() method rather than __hash__ and then a wrapper class that treats polynomials as immutable can do:
def __hash__(self):
return self._rep.hash()
Sorry, something went wrong.
There was a problem hiding this comment.
Perhaps there could be explicit methods like p1.imul(p2) for in-place mutation.
Have just added explicit iadd, isub, and imul methods based off the previous in-place methods.
Sorry, something went wrong.
This reverts commit 8f76c6a.
|
Okay, I think this looks good. I'll let the tests finish and then merge. Thanks for keeping with it through the lengthy reviews. It is always tricky when designing new interfaces. We need to be sure that we will be happy with the design for many other mpoly types in future. |
Sorry, something went wrong.
|
Okay, looks good. Thanks Jake! |
Sorry, something went wrong.
|
I've pushed 0.7.0a3 to PyPI with the multivariate polynomials included: |
Sorry, something went wrong.
Completely agree, thank you for all of your diligent responses and reviews.
🎉 |
Sorry, something went wrong.
I've merged master into #59 and have started pouring over the changes. I've done a small refactor of the context cache to just get myself acquainted with the code base more than anything else. Not married to those changes. I've also written up a small state of the PR.
This PR aims to supersede #59.
State of the PR
Existing files and flintlib completeness
Here's all the modified files from this PR
Of these, there are some build/meta related or unimportant changes, these are
The changes that are of substance, in my opinion, are
With three of the remaining being unsubstantial
The remaining file is an odd one, this is a duplicate file name, but it's contents differ significantly from src/flint/types/fmpq_mpoly.pyx. It's also an odd one out with the project structure. The other file makes references to fmpz_mpoly, leading me to believe it is a scratch file. src/flint/types/fmpq_mpoly.pyx is much newer and src/flint/fmpq_mpoly.pyx only appears in one commit.
From this, the scope of this PR seems to be
Add Cython declarations of the C functions for
fmpq_mpoly.h,
fmpq_mpoly_factor.h,
fmpz_mpoly_factor.h,
and fmpz_mpoly_q.h,
python-flint already includes support for fmpz_mpoly.h.
It also appears to contain a small refactor of the existing fmpz_mpoly modules to account for the new mpoly's.
Using a couple snippets of elisp to assess the completeness of the flintlib files based on the publicly documented functions
I think that's quite good, those functions doesn't seem necessary, and any new ones will be easy to add on demand. I've done similar with all functions however it's quite long, and in my opinion, not of any use.
Outstanding comments
Both call methods for fmpz_mpoly and fmpq_mpoly are missing evalutation methods (__call__), some existing code is there, however commented out.
#59 (comment)
I'm also yet to look at @deinst's 6 commits on their initialize_fmpz branch, their other branches appear to be up to date.
I'll also aim to give an assessment of the outstanding comments on the old PR, I don't believe they are resolved.
MVP
When should we this PR to be complete? The core components of fmpz_mpoly, fmpq_mpoly, and fmpz_mpoly_q appear to be near complete, just lacking evaluation and the comments to be resolved. However, no work as been started on both of the factor modules, although the flintlib declarations exist.