Interface for making an mpoly context

[oscarbenjamin]

CC @Jake-Moss

The idea of higher-level interfaces is discussed in gh-53. I would imagine that ultimately the usual way to create a multivariate polynomial would look something like:

from flint import ZZ
R, [x, y] = ZZ.mpoly_ring(['x', 'y'])
p = x**2 - y**2
p.factor()

Other options like monomial ordering could be keyword arguments.

In that sense we can think of the current interface as being sort of "low-level" but still I think it can be nicer than it is. This is currently the way to construct and use a multivariate polynomial context:

ctx = fmpz_mpoly_ctx(3, Ordering.lex, ["x", "y", "z"])
x, y, z = ctx.gens()
(x**2 - y**2).factor()

I think it is reasonable that the generators are not returned by default but the constructor syntax is awkward. It is redundant to specify the number of generators if we are already supplying a list of names. Also most of the time using lex as the default ordering is fine. I doubt many people will ever care about the monomial ordering.

It would be better to change it so that you can do

ctx = fmpz_mpoly_ctx(["x", "y", "z"])

and get a lex ordered ring by default. I think it would be reasonable to say that the ordering parameter could be a keyword-only argument like:

ctx = fmpz_mpoly_ctx(["x", "y", "z"], order='degrevlex')

Another problem is that the monomial ordering has to be passed as an enum which is also awkward e.g.:

import flint
ctx = flint.fmpz_mpoly_ctx(["x", "y", "z"], order=flint.Ordering.degrevlex)

It is reasonable to use these enums but it should be possible to just pass a string when calling the constructor. I think that a string should be displayed in the repr as well:

>>> ctx
fmpz_mpoly_ctx(["x", "y", "z"], order="lex")

The reason that the first argument to the current constructor is an integer is because it is envisaged that you would do:

>>> ctx = fmpz_mpoly_ctx(3, Ordering.lex, "x")
>>> ctx.gens()
(x1, x2, x3)

I think that a better way of handling that is to allow a symbol name to be a tuple of name and count e.g.:

>>> ctx = fmpz_mpoly_ctx([("x", 3), "y"])
>>> ctx.gens()
(x1,x2,x3,y)

The docstring for fmpz_mpoly_ctx says not to call it directly. I believe this is because get_context manages a cache of contexts. That is reasonable but in that case I think that calling the constructor directly should give an error and then my comments above apply to get_context as well.

The interface for get_context is:

In [11]: ctx = fmpz_mpoly_ctx.get_context(3, Ordering.lex, names="x")

In [12]: ctx.gens()
Out[12]: (x0, x1, x2)

In [14]: ctx = fmpz_mpoly_ctx.get_context(3, Ordering.lex, nametup=("x", "y", "z"))

In [15]: ctx.gens()
Out[15]: (x, y, z)

Again I think that the argument are somewhat backwards in:

fmpz_mpoly_ctx.get_context(3, Ordering.lex, nametup=("x", "y", "z"))

Ideally it should be

fmpz_mpoly_ctx.get_context(["x", "y", "z"])

The name get_context is a bit confusing for a method that creates a new context. I would perhaps rename that to new like:

fmpz_mpoly_ctx.new(["x", "y", "z"])

Also the parameter names are strange because names plural is what is used when passing a single string and nametup is what I would have naturally called names. The tup emphasises that it needs to be a tuple but really it should be okay to have a list here:

>>> ctx = fmpz_mpoly_ctx.get_context(3, Ordering.lex, nametup=["x", "y", "z"])
...
TypeError: Argument 'nametup' has incorrect type (expected tuple, got list)

I'm not sure we need the names and nametup arguments at all though. This is how I think I would do it:

ctx = fmpz_mpoly_ctx.new(["x", "y", "z"])  # x,y,z
ctx = fmpz_mpoly_ctx.new([("x", 2)])   # x1,x2
ctx = fmpz_mpoly_ctx.new([("x", 2), y])  # x1,x2,y

Another option seen in SymPy is:

In [17]: symbols('x:3, y')
Out[17]: (x0, x1, x2, y)

That looks nice in some situations but it is actually awkward when the number of symbols is a variable because you have to format it into a string:

In [18]: n = 3

In [19]: symbols(f'x:{n}, y')
Out[19]: (x0, x1, x2, y)

Also I think it is better to keep things simple at this level and avoid any sort of string parsing.

[oscarbenjamin]

Another thing I notice is that there are no docs for the mpoly types:
https://python-flint.readthedocs.io/en/latest/

[Jake-Moss]

Thanks for your patience with this.

The idea of higher-level interfaces is discussed in gh-53. I would imagine that ultimately the usual way to create a multivariate polynomial would look something like:

from flint import ZZ
R, [x, y] = ZZ.mpoly_ring(['x', 'y'])
p = x**2 - y**2
p.factor()

I agree, this would be a nicer interface for interacting with python-flint.

In that sense we can think of the current interface as being sort of "low-level" but still I think it can be nicer than it is. This is currently the way to construct and use a multivariate polynomial context:

ctx = fmpz_mpoly_ctx(3, Ordering.lex, ["x", "y", "z"])
x, y, z = ctx.gens()
(x**2 - y**2).factor()

Completely agree, it is rather clunky at the moment, and is really just a holdover from the original mpoly PR. I'm looking at starting a revamp on this tonight, though I do have to shift focus to getting a POC of the mpoly types working well in SymPy soon.

I think that a better way of handling that is to allow a symbol name to be a tuple of name and count e.g.:

>>> ctx = fmpz_mpoly_ctx([("x", 3), "y"])
>>> ctx.gens()
(x1,x2,x3,y)

I quite like this, nice and simple.

Another problem is that the monomial ordering has to be passed as an enum which is also awkward e.g.:

import flint
ctx = flint.fmpz_mpoly_ctx(["x", "y", "z"], order=flint.Ordering.degrevlex)

This should be easy to change. The built in enum.Enum class can handle this nicely. The cython interface via cdef enum is limited to the most basic definitions.

Also the parameter names are strange because names plural is what is used when passing a single string and nametup is what I would have naturally called names. The tup emphasises that it needs to be a tuple but really it should be okay to have a list here:

>>> ctx = fmpz_mpoly_ctx.get_context(3, Ordering.lex, nametup=["x", "y", "z"])
...
TypeError: Argument 'nametup' has incorrect type (expected tuple, got list)

Yeah this is because of the type hints being enforced by Cython. It's not something that I've found spelled out the documentation in bold letters but the annotation_typing compiler directive defaults to True and the reference table for the python mode says that built in types are enforced exactly.
https://docs.cython.org/en/latest/src/tutorial/pure.html#reference-table

I believe the particular type hint was to specificy the argument should be both hashable and iterable. AFAIK this isn't supported by the collections.abc type hints

In [2]: from collections.abc import Hashable, Iterable

In [3]: names: Hashable[Iterable[str]]
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[3], line 1
----> 1 names: Hashable[Iterable[str]]

TypeError: type 'Hashable' is not subscriptable

And an intersection type is not currently in python. There seems to be some effort on this though https://github.com/CarliJoy/intersection_examples. I don't think this is really an issue though as this will be hidden by the new interface. It will instead be Iterable[Hashable], and then stored as a tuple regardless of the input iterable.

The name get_context is a bit confusing for a method that creates a new context. I would perhaps rename that to new like:

fmpz_mpoly_ctx.new(["x", "y", "z"])

Agreed, there is nothing to "get" initially. Although new might imply that the returned context is a new object but I think that can just be spelled out in the doc string.

[Jake-Moss]

Just found a minor inconsistency in the mpoly_ctx.any_as_scalar functions I wanted to comment on. My motivation for it was to have a single function to create a scalar in the form of a common denominator for that type, i.e. fmpz for fmpz_mpoly, fmpq for fmpq_mpoly, ulong for nmod_mpoly. This was to simplify the arithmetic operators and avoid having multiple if-else ladders checking the type, or having multiple functions for each type. The Flint implementions turn the alternative types into the arbitrary sized ones anyway.

What I've found is that since the fmpz_mod_mpoly functions accept a fmpz_t rather than a fmpz_mod_t, I wrote the any_as_scalar function to return an fmpz. But that seems a little miss-leading, additionally it errors on mismatching modulus, but never actually uses it.

So for all mpoly types I'll also turn the existing function into a hidden one for use internally, and exposing a replacement that considers the modulus when appropriate. As I've got this locally it's messier than I would have liked. I'll revisit it later

[Jake-Moss]

I'm also looking at implementing some functions to make working with and between contexts a little nicer, in particular for the DMP_Flint integration in sympy. I'm planning on adding functions to increase/decrease the number of generators as well as an interface to the *_mpoly_compose_*_mpoly_gen functions for going between contexts.

As flint separates generators and their names, I think python-flint could as well, eventually moving it to the "high level" domain interface, and the "low level" existing functions should only have to consider indices. For example

from flint import ZZ
# generator names are designated here
R, [x, y] = ZZ.mpoly_ring(['x', 'y'])

# and the "low level" interface only requires specifying the number of variables. 
ctx = fmpz_mpoly_ctx(2, order='lex')

As it stands the contexts are identified by the nametup in addition to nvars and ordering. This isn't strictly required.

One issue I see with this separation is that printing the low level types becomes a little cumbersome as the user would have to specify variable names, or some defaults would have to be used. I'm not sure how this could be solved without introducing wrapper types and printing machinery

[oscarbenjamin]

Agreed, there is nothing to "get" initially. Although new might imply that the returned context is a new object but I think that can just be spelled out in the doc string.

Yeah, maybe "get" is better. Neither is really accurate in all cases.

One issue I see with this separation is that printing the low level types becomes a little cumbersome as the user would have to specify variable names, or some defaults would have to be used. I'm not sure how this could be solved without introducing wrapper types and printing machinery

I would go the other way and add variable names to all univariate and multivariate polynomials at the low level. It is not just about printing but also conversions between different contexts when there are symbols.

[oscarbenjamin]

I do have to shift focus to getting a POC of the mpoly types working well in SymPy soon.

Yes, fair enough.

To be clear though, the reason I haven't yet put out a release of python-flint even though lots of new features have been added is that I'm not sure about the call signature for constructing an mpoly context. If that is going to be changed then it should be changed before the next release.

The other reason I haven't yet put out a release is because I imagine that the SymPy integration is going to result in some changes to the mpoly types/interface as well.

In my mind the sequence is:

1. Make the mpoly context constructors good.
2. Put out a prerelease of python-flint (this will help with testing on SymPy side).
3. Get the SymPy integration working at least at POC level with tests passing (and see if more changes needed in python-flint).
4. Make a full release of python-flint.
5. Finalise the SymPy integration.

[Jake-Moss]

Agreed, there is nothing to "get" initially. Although new might imply that the returned context is a new object but I think that can just be spelled out in the doc string.

Yeah, maybe "get" is better. Neither is really accurate in all cases.

I think "get" works, it partially mirrors the dict.get behaviour of returning an existing object, or some default one

[Jake-Moss]

I believe everything mentioned here other than the high level interface and context tools are implemented in the linked PR

[oscarbenjamin]

Closing after gh-225