Start adding fq_default type

[GiacomoPope]

I did this work a few weeks ago, then dropped it because I got married and now I'm trying to implement other work for a paper so my brain is VERY outside of python-flint world but thought I'd show at least roughly what's happening for this type and what we'll need.

Main thing is that we have these "union structs" for the fq_default type which aren't implemented in cython, so we need to do nesting instead.

[oscarbenjamin]

Note also previous discussion about this in gh-91.

[oscarbenjamin]

The way that you have defined the fq_default* structs in Cython here seems reasonable to me. I'm actually unsure if it is necessary to define anything about these structs at all if only fq_default_* functions are being used. Maybe these structs can just be treated as opaque at the Cython level. If there is any need to convert them then maybe functions like fq_default_get_fmpz_mod_poly are sufficient.

I think it is only necessary to define the struct members if we want to use direct member access but if the fq_default* functions are used instead then it is probably fine (possibly safer?) to just leave the struct definitions empty.

[GiacomoPope]

After I've wrapped up a few other projects, I'll come back to this then and at least get the fq_default doing some standard stuff. Then we can maybe start looking at adding the generic types in

[oscarbenjamin]

It's always worth remembering that it is most important just to get the basic features in. Trying to add all the possible functions that finite fields can support can take a lot longer than just making it possible to construct the objects in the first place and do some basic arithmetic.

Also once a type is added it becomes a lot easier for other people in future to add specific features if they want them.

[GiacomoPope]

The topic of this work came up at a Sage days I am currently attending and with some of my research code finished I now have more time to start working more on this again. Latest commit was simply merging master back into this branch to be up to date with the work I've missed.

[GiacomoPope]

@oscarbenjamin I'm being stupid with the local build...

If I run:

source bin/activate
./bin/build_inplace.sh   

I get an error from the current setup:

Traceback (most recent call last):
  File "/Users/Jack/Documents/GitHub/python-flint/setup.py", line 143, in <module>
    ext_modules=cythonize(ext_modules, compiler_directives=compiler_directives),
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/Jack/Library/Python/3.12/lib/python/site-packages/Cython/Build/Dependencies.py", line 1010, in cythonize
    module_list, module_metadata = create_extension_list(
                                   ^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/Jack/Library/Python/3.12/lib/python/site-packages/Cython/Build/Dependencies.py", line 845, in create_extension_list
    for file in nonempty(sorted(extended_iglob(filepattern)), "'%s' doesn't match any files" % filepattern):
  File "/Users/Jack/Library/Python/3.12/lib/python/site-packages/Cython/Build/Dependencies.py", line 117, in nonempty
    raise ValueError(error_msg)
ValueError: 'src/flint/types/fmpz_mpoly_q.pyx' doesn't match any files

At the moment, I do testing by running

pip install .

and then working directly with this :)

Is this now "best practice" for development? The meson stuff has changed the internals since i last worked on python-flint

[oscarbenjamin]

Yes, it has changed because of the meson stuff which I need to properly document somewhere.

The setup.py is no longer needed but is just there for compatibility. The build_inplace script uses setup.py so should not be used any more. In meson there is no concept of an in-place build because all builds are always out-of-tree in a build directory.

There are several ways of doing this now:

First way:

Use something like pip as a build frontend which will invoke the meson-python build backend (see PEP 517). That means that e.g. pip install . will work.

You can also use this to create a meson-python editable install like:

pip install meson meson-python cython
pip install --no-build-isolation --editable .

This editable install will be built in the ./build directory. Every time you import flint from Python it will trigger an incremental rebuild.

Second way:

Don't bother with pip and the whole PEP 517 business. Run meson commands directly to build things instead. First get a clean setup:

pip uninstall python-flint
rm -r build build-install

Now think of these commands as ./configure, make and then make install:

meson setup build --pkg-config-path=$(pwd)/.local/lib/pkgconfig
meson compile -C build
meson install -C build --destdir ../build-install

Now the build-install directory looks like this:

$ tree build-install/
build-install/
└── usr
    └── local
        └── lib
            └── python3.11
                └── site-packages
                    └── flint
                        ├── flint_base
                        │   ├── flint_base.cpython-311-x86_64-linux-gnu.so
                        │   ├── flint_context.cpython-311-x86_64-linux-gnu.so
                        │   ├── __init__.py
                        │   └── __pycache__
                        │       └── __init__.cpython-311.pyc
...

Add that directory to PYTHONPATH and run the tests:

export PYTHONPATH=$(pwd)/build-install/usr/local/lib/python3.11/site-packages
export LD_LIBRARY_PATH=$(pwd)/.local/lib
python -m flint.test

Third way:

Running the meson commands directly is tedious so spin (pip install spin) acts as a frontend to make this nicer. Make sure you have a clean setup (no editable install, etc) first:

pip uninstall python-flint
rm -r build build-install

Now spin can build and run the tests for you. If you have flint installed system wide (rather than in a local directory) then it should be as simple as running spin test which will build everything, install it into build-install, add to PYTHONPATH and run all the tests. This builds in parallel and uses incremental rebuilds so it's a lot faster.

I don't have Flint installed system wide so I need to configure the build first like this:

spin build -- --pkg-config-path=$(pwd)/.local/lib/pkgconfig -Dadd_flint_rpath=true

That configuration is persistent so now I can just run e.g. spin test to rebuild and run tests.

Fourth way:

It is also possible with spin to use an editable install and spin install will set that up for you by running pip. Note that spin is just a convenience frontend. It prints out all of the commands that it executes in case you want to run them yourself with different arguments or something.

Note that these different ways are not compatible with each other so you have to choose one. You probably need a deep clean when switching from one way to another because they use the build directory in incompatible ways which is why I recommend:

pip uninstall python-flint
rm -r build build-install

I'm not sure what should be the recommended way right now. Probably using spin without an editable install should be recommended although the editable install is quite convenient. I have spent a long time learning how all the pieces work and so I find all of these things easy to use now but I'm not sure what others would prefer. I think @Jake-Moss prefers to use spin without an editable install.

With meson all building is out of tree so if you have an old checkout where you have been doing in-place builds then it is probably best to clean out all the old .c etc files with git clean. The meson build will not generate those files in the source tree any more.

[GiacomoPope]

AMAZING! Thanks so much for such a detailed answer :)

I'll work at finally getting this type done this evening

[GiacomoPope]

@oscarbenjamin this is still very much unfinished, but I wanted to point to the arithmetic boilerplate code in the flint_scalar class -- is this something valuable? I thought it might be worth having another PR / issue where we discuss some generic functions which we think should be included for all these base classes as a guide for people implementing new types?

This links again to what you said in the other PR with pow_trunc -- having expectations with boilerplates in the base class could help?

[oscarbenjamin]

I wanted to point to the arithmetic boilerplate code in the flint_scalar class -- is this something valuable?

Yes, I think so.

I think that for some very basic types like nmod, fmpz, etc it is important for the subclass to implement most/all of its own boiler-plate for maximum speed when e.g. adding nmod + nmod or fmpz + int. For most other types like polynomials, matrices etc it matters much less because each operation is generally more expensive. Cython's subclass v-tables are a fast dispatch mechanism in my experience so the overhead of splitting the implementation of a method between super and subclass is quite small for cdef classes.

In general though I think it would be better if most types could inherit something that manages most of the __add__ etc boiler-plate like checking compatible types, coercing int etc. As you say it would help with defining what the expected methods should be as well.

I thought it might be worth having another PR / issue where we discuss some generic functions which we think should be included for all these base classes as a guide for people implementing new types?

I think that makes sense.

[Jake-Moss]

I think @Jake-Moss prefers to use spin without an editable install.

Yep that's correct! IIRC the editable install doesn't support prompting a compilation from anything other than importing. This didn't jell with me, I prefer my in-editor write, compile, test workflow with an explicit (or automatic) compilation step.

However, not using an editable install means all my python-flint related tools need to be aware of spin to properly setup PYTHON_PATH, so I configure my emacs setup with the Python executable name as spin with the necessary options. This means I can have an explicit compilation step (with my choice of arguments) if I desire it via spin build, and any invocation of spin prompts an incremental build. So things like launching pytest or a python repl first call spin, building if necessary, then running as normal.

IMO it's pretty close to optimal, only thing left for me to tinker with is to write some spin layout for direnv and drop the emacs specific configuration, but prefixing things with spin works well enough for now.

[oscarbenjamin]

IMO it's pretty close to optimal

Okay, so maybe I'll just write some docs that explain how to use spin like that. I can give some context for how to set things up the other ways but it is good to have a guide that clearly describes one way of doing things.

Another thing I'm not sure about:

Do either of you build against a system install of Flint or use a local build?

I always use a local build in $(pwd)/.local because I want to have local control over which version of Flint is being used. The bin/build_dependencies_unix.sh script sets up the local build of GMP, MPFR and Flint to make this work. Do you use that script?

Using a local build complicates things a bit because you need to fiddle with environment variables. Hence the bin/activate script: https://github.com/flintlib/python-flint/blob/069d24d5d7a85bb8e264359ca9f3fcc8e744921d/bin/activate#L1-L3

With spin/meson there is a better way than using environment variables but it just complicates exactly the instructions that are needed for setting up the development build. Firstly meson uses pkgconfig to locate dependencies like Flint and so rather than setting LIBRARY_PATH or C_INCLUDE_PATH you set PKG_CONFIG_PATH which you can do with the --pkg-config-path command line argument. That needs to be done in the call to meson setup or spin build. With spin this means that you need to have a first explicit call to spin build before you can then use e.g. spin test because arguments like --pkg-config-path cannot be passed to spin test (its arguments are all passed to pytest rather than to meson).

Setting --pkg-config-path means that the build completes but it is also necessary to be able to find the libflint shared library at runtime. That is what LD_LIBRARY_PATH is for on Linux (or DYLD_LIBRARY_PATH on macos or PATH on Windows). At least on Linux or macos there is a better way than using environment variables which is to add an rpath entry. The meson-python editable install does this automatically. When using spin it happens automatically on macos but not on Linux. I've added a meson configuration option called add_flint_rpath for this. Possibly it makes sense to have this option on by default somehow but this is needed as well.

This is all why for me the first command I need to run is either of:

meson setup build --pkg-config-path=$(pwd)/.local/lib/pkgconfig -Dadd_flint_rpath=true
spin build -- --pkg-config-path=$(pwd)/.local/lib/pkgconfig -Dadd_flint_rpath=true

It might be handy to put this into a convenient script like bin/setup_spin.sh or something so then the instructions to set up a dev checkout are:

git clone [email protected]:flintlib/python-flint.git
cd python-flint
pip install -r requirements-dev.txt
bin/build_dependencies_unix.sh
bin/setup_spin.sh

And then spin test, spin docs, spin sdist etc should work. I think one thing missing right now is spin doctest.

I'm not sure though if I can assume though that:

1. Everyone wants to use a local build of Flint when working on python-flint.
2. The path $(pwd)/.local/lib/pkgconfig is always correct. It might be lib64 or something on some systems.
3. It is always fine to add the rpath entry. I think at least if this is just advice for a dev checkout then probably it is fine. There are apparently reasons why meson does not do this by default (https://github.com/mesonbuild/meson/issues/13046).

[GiacomoPope]

@oscarbenjamin small issue with doc tests -- the default type picked between 3.0 and 3.1 seems to be different, which means I can't get things to pass for both...

One option would be simply to not have the context type included on the __repr__

[oscarbenjamin]

the default type picked between 3.0 and 3.1 seems to be different, which means I can't get things to pass for both...

Could just mark the doctest to be skipped.

One option would be simply to not have the context type included on the __repr__

It might be nicer if it displayed as something other than just an integer. The multivariate polynomials use an enum for monomial orderings. Perhaps something similar could be used here:

In [1]: import flint

In [2]: ctx = flint.fmpz_mpoly_ctx(3, flint.Ordering.lex, ["x", "y", "z"])

In [3]: ctx
Out[3]: fmpz_mpoly_ctx(3, '<Ordering.lex: 0>', ('x', 'y', 'z'))

I might prefer that to just show as this and for this to be valid though:

fmpz_mpoly_ctx(3, 'lex', ('x', 'y', 'z'))

Using integers makes sense in C but in Python strings are nicer. Enums are also good but it is nice if you can shortcut them with something like 'lex' that is still meaningful.

[Jake-Moss]

Do either of you build against a system install of Flint or use a local build?

I've been using something pretty similar with a local build, just in a different location. I've controlled everything via a .envrc where I set PKG_CONFIG_PATH, LIBRARY_PATH, LD_LIBRARY_PATH, and C_INCLUDE_PATH appropriately. Very recently I've switched to using a Nix flake to manage everything where I can override various options without building from source manually. Having pkg-config with nix is enough that I don't need to configure anything else.

2. The path $(pwd)/.local/lib/pkgconfig is always correct. It might be lib64 or something on some systems.

My certainly uses (or rather used) lib64.

3. It is always fine to add the rpath entry. I think at least if this is just advice for a dev checkout then probably it is fine. There are apparently reasons why meson does not do this by default (https://github.com/mesonbuild/meson/issues/13046).

After having read that issue I think it's probably fine for a quick automated install script to add the rpath entry if they have not installed flint via a package manager. It's always possible for contributors to do things themselves if anything goes wrong.

[GiacomoPope]

Amazing. I'll work on this this evening if I have time. For coverage, how best should I check this? coverage run ?

[Jake-Moss]

It might be nicer if it displayed as something other than just an integer. The multivariate polynomials use an enum for monomial orderings. Perhaps something similar could be used here:

In [1]: import flint

In [2]: ctx = flint.fmpz_mpoly_ctx(3, flint.Ordering.lex, ["x", "y", "z"])

In [3]: ctx
Out[3]: fmpz_mpoly_ctx(3, '<Ordering.lex: 0>', ('x', 'y', 'z'))

I might prefer that to just show as this and for this to be valid though:

fmpz_mpoly_ctx(3, 'lex', ('x', 'y', 'z'))

Using integers makes sense in C but in Python strings are nicer. Enums are also good but it is nice if you can shortcut them with something like 'lex' that is still meaningful.

I should be able to fix that by using a Python based enum.Enum class instead of the cpdef enum as we can then specify strings as the constants. Not sure why I opted for the cython-based class over the Python one in the first place. I've been slacking on https://github.com/flintlib/python-flint/pull/164, I can make the change after that.

[oscarbenjamin]

For coverage, how best should I check this? coverage run ?

I'm not sure. This is one thing that does not currently work with meson because Cython gets confused about the out of tree build. I showed (in https://github.com/cython/cython/issues/6186#issuecomment-2104842388) how to patch Cython and then add something to python-flint's coverage config that would make it work I think.

Jake have you managed to measure coverage with the meson build somehow?

[oscarbenjamin]

I should be able to fix that by using a Python based enum.Enum class instead of the cpdef enum as we can then specify strings as the constants.

I don't think it needs the Python enum. Is it not just a case of checking for a string in the constructor and then also displaying a string in the repr?

[GiacomoPope]

In [18]: fq_default_ctx(5, 2, fq_type=1)
Out[18]: fq_default_ctx(5, 2, 'x', x^2 + 4*x + 2, 'FQ_ZECH')

In [19]: fq_default_ctx(5, 2, fq_type=2)
Out[19]: fq_default_ctx(5, 2, 'x', x^2 + 4*x + 2, 'FQ_NMOD')

In [20]: fq_default_ctx(5, 2, fq_type=3)
Out[20]: fq_default_ctx(5, 2, 'x', x^2 + 4*x + 2, 'FQ')

I have this set up, where I convert from a string if necessary in the __init__ and print as a string in the __repr__

[oscarbenjamin]

After having read that issue I think it's probably fine for a quick automated install script to add the rpath entry if they have not installed flint via a package manager.

I'm not sure that we can check how Flint was installed. I think that's what the discussion in the linked issue came down to i.e. that it is not easy to figure out if the rpath is needed or not.

Currently there is a project option add_flint_rpath and there are a number of different ways to set it:

meson setup build --pkg-config-path=$(pwd)/.local/lib/pkgconfig -Dadd_flint_rpath=true
spin build -- --pkg-config-path=$(pwd)/.local/lib/pkgconfig -Dadd_flint_rpath=true
pip install . --config-settings=setup-args=--pkg-config-path=$(pwd)/../sympy/.local/lib/pkgconfig --config-settings=setup-args=-Dadd_flint_rpath=true

I can just list those commands in the instructions somewhere and let people figure it out. There's no way round having someone specify the pkgconfig location. I think for pip install --editable . or spin install the rpath gets set anyway because meson-python has a fixer that sets it but only in an editable install.

I suspect that this is only ever needed for development work anyway because otherwise Flint will be installed system-wide or there will be some other system for making it available like with environment variables. For the binary wheels there is no issue because the cibuildwheel repair step bundles all the libs and fixes all the rpaths.

[GiacomoPope]

I'm sure this is not perfect, but it's a good start at exposing most of the functionality of the fq_default type. Ready for review

[Jake-Moss]

Jake have you managed to measure coverage with the meson build somehow?

No I have not, I believe it blocked by the linked issue. If your patch is working I could automate the patching of cython via nix but I don't believe that is a nice solution. To get my coverage reports I have a separate copy of python-flint that I build with setup.py.

I can just list those commands in the instructions somewhere and let people figure it out.

Sorry this is what I meant. You can only do so much, I think documenting it is enough

I should be able to fix that by using a Python based enum.Enum class instead of the cpdef enum as we can then specify strings as the constants.

I don't think it needs the Python enum. Is it not just a case of checking for a string in the constructor and then also displaying a string in the repr?

We certainly can, I believe I added it here https://github.com/flintlib/python-flint/pull/132/commits/17b3406758855ee3028c7f433c4d86442f872551 because I just wanted to move the ordering specific stuff out and away from the context classes. It would be very easy to just revert to using strings

[GiacomoPope]

The main thing missing now is probably thorough coverage of all the branching for context creation... I'm not sure if I'm even 100% happy with the current context generation but it's a start

[GiacomoPope]

@fredrik-johansson do you know if there's a plan to make / include a fq_default_embed -- or should we have bindings for the three separate cases in python-flint

[oscarbenjamin]

if there's a plan to make / include a fq_default_embed -- or should we have bindings for the three separate cases in python-flint

It should be possible to just convert fq_default to fq and use the fq_embed functions. Then any returned fq can be converted back to the given fq_default contexts. The polynomials or matrices involved can use fmpz_mod although perhaps conversion to/from nmod could be used for the fq_nmod/fq_zech cases.

[GiacomoPope]

I'd be interested if the fq_default_embed was planned though, as this limits the amount of internal python conversion needed?

Similarly, following: https://github.com/flintlib/flint/issues/2045 it would be nicer for C to do the checking than python, but we could do this our side too

[oscarbenjamin]

it would be nicer for C to do the checking than python, but we could do this our side too

Until we raise the minimum supported Flint version (currently 3.0) then we will need to have an implementation of these things in python-flint even if the functions are added to future versions of Flint.

Raising the minimum supported Flint version would simplify some things. On the other hand it has only just in the last few months become possible to build python-flint against a distro supplied build of libflint which is nice: https://github.com/flintlib/python-flint/blob/c4847e72fcc31c6d1f56653935705679cffa3d1d/.github/workflows/buildwheel.yml#L123-L139 It might be that no distros ship Flint 3.0 rather than 3.1 though in which case 3.1 would be a better baseline. Most version conditional code is for Flint < 3.1 and could be removed if Flint 3.0 was dropped: https://github.com/flintlib/python-flint/blob/c4847e72fcc31c6d1f56653935705679cffa3d1d/meson.build#L33-L39 https://github.com/flintlib/python-flint/blob/c4847e72fcc31c6d1f56653935705679cffa3d1d/src/flint/flintlib/fmpz_mod_mat.pxd#L21-L28

[oscarbenjamin]

It might be that no distros ship Flint 3.0 rather than 3.1 though in which case 3.1 would be a better baseline.

Another consideration is what version of Flint is used in Sage. I think python-flint is part of Sage now because it is used internally by SymPy and perhaps will be by mpmath in future. I assume that it is useful for Sage if python-flint has a bit of flexibility about which version of Flint it supports.

[GiacomoPope]

Attempting to resolve the discussion over equality and coercion I have written the above