Adding Fmpz mpoly #59, cont. #132
Conversation
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
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. |
|
Wonderful to hear. I've always thought of a new
I'm sure we will as well. |
|
You might find it helpful to read this: |
src/flint/flint_base/flint_base.pyx
Outdated
| 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.
There was a problem hiding this comment.
Now that we have monoms and coeffs which perform the same role.
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
src/flint/types/fmpq_mpoly.pyx
Outdated
| 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]: TrueThere 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
monomialreturns a tuple of exponent values, not a monomial in the form of afmp[zq]_mpoly.monomsreturns a list of tuples of exponent values, not a sequence of monomials in the form offmp[zq]_mpolys.termsreturns a zip of coefficients and tuples of exponent values, not a sequence of terms in the form offmp[zq]_mpolys.
Given that the flint interface provides easy methods for this I think that they would be reasonable functions to have do so
src/flint/types/fmpz_mpoly.pyx
Outdated
| 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.
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.
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.
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.
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()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.
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. |
|
Okay, looks good. Thanks Jake! |
|
I've pushed 0.7.0a3 to PyPI with the multivariate polynomials included: |
Completely agree, thank you for all of your diligent responses and reviews.
🎉 |
I've merged
masterinto #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
flintlibcompletenessHere'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 tofmpz_mpoly, leading me to believe it is a scratch file.src/flint/types/fmpq_mpoly.pyxis much newer andsrc/flint/fmpq_mpoly.pyxonly 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-flintalready includes support forfmpz_mpoly.h.It also appears to contain a small refactor of the existing
fmpz_mpolymodules to account for the newmpoly's.Using a couple snippets of elisp to assess the completeness of the
flintlibfiles based on the publicly documented functionsI 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_mpolyandfmpq_mpolyare 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, andfmpz_mpoly_qappear to be near complete, just lacking evaluation and the comments to be resolved. However, no work as been started on both of thefactormodules, although theflintlibdeclarations exist.