Include the fq_default type
#97
Conversation
|
Note also previous discussion about this in gh-91. |
|
The way that you have defined the I think it is only necessary to define the struct members if we want to use direct member access but if the |
|
After I've wrapped up a few other projects, I'll come back to this then and at least get the |
|
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. |
|
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. |
|
@oscarbenjamin I'm being stupid with the local build... If I run: I get an error from the current setup: At the moment, I do testing by running 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 |
|
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 You can also use this to create a meson-python editable install like: This editable install will be built in the Second way: Don't bother with Now think of these commands as Now the $ 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.testThird way: Running the meson commands directly is tedious so spin ( Now I don't have Flint installed system wide so I need to configure the build first like this: That configuration is persistent so now I can just run e.g. Fourth way: It is also possible with 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: I'm not sure what should be the recommended way right now. Probably using 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 |
|
AMAZING! Thanks so much for such a detailed answer :) I'll work at finally getting this type done this evening |
|
@oscarbenjamin this is still very much unfinished, but I wanted to point to the arithmetic boilerplate code in the This links again to what you said in the other PR with |
Yes, I think so. I think that for some very basic types like In general though I think it would be better if most types could inherit something that manages most of the
I think that makes sense. |
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 IMO it's pretty close to optimal, only thing left for me to tinker with is to write some |
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 Using a local build complicates things a bit because you need to fiddle with environment variables. Hence the Lines 1 to 3 in 069d24d 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 Setting This is all why for me the first command I need to run is either of: It might be handy to put this into a convenient script like And then I'm not sure though if I can assume though that:
|
|
@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 |
Could just mark the doctest to be skipped.
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 |
I've been using something pretty similar with a local build, just in a different location. I've controlled everything via a
My certainly uses (or rather used)
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 |
|
Amazing. I'll work on this this evening if I have time. For coverage, how best should I check this? |
I should be able to fix that by using a Python based |
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 cython/cython#6186 (comment)) 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? |
I tried this but I couldnt get cython right to find out how to return the pointer as a C-type -- i get errors about it trying to convert the type to a Python object however i refactor it |
|
The above is also a nice idea -- i need a break from this but if you want, please add this change (either as a diff or a commit) -- im a little cython confused atm :D |
Fair enough maybe to make things easier we can just postpone adding |
src/flint/flint_base/flint_base.pyx
Outdated
| other = self._any_as_self(other) | ||
| if other is NotImplemented: | ||
| return NotImplemented | ||
| return self._sub_(other, swap=True) |
There was a problem hiding this comment.
Rather than having swap=True I would just have two methods _sub_ and _rsub_.
There was a problem hiding this comment.
The idea is that __sub__ handles all conditionality so that _sub_ and _rsub_ do not need to check anything. Also keyword arguments can add overhead that I would avoid at this level.
There was a problem hiding this comment.
OK I've done this for the base class and added rsub, rdiv.
Maybe we can circle back to this when we deal with |
src/flint/types/fq_default.pyx
Outdated
| def _add_(self, other): | ||
| """ | ||
| Assumes that __add__() has ensured other is of type self | ||
| """ | ||
| cdef fq_default res | ||
| res = self.ctx.new_ctype_fq_default() | ||
| fq_default_add(res.val, self.val, (<fq_default>other).val, self.ctx.val) | ||
| return res |
There was a problem hiding this comment.
I would add types for the parameters here and make this cdef I think:
cdef fq_default _add_(fq_default self, fq_default other):
cdef fq_default res = self.ctx.new_ctype_fq_default()
fq_default_add(res.val, self.val, other.val, self.ctx.val)
return resThere was a problem hiding this comment.
I'm not sure if making this cdef makes much difference though.
There was a problem hiding this comment.
I had to use cpdef instead of cdef -- is this OK? I suppose this is needed to return the python object
There was a problem hiding this comment.
I'm not sure what is the best way to make this work. If we want to use this widely though (for other types) then it would be good to benchmark, look at the generated C code, try alternatives etc.
I am concerned about the overhead of these methods. See e.g.: sympy/sympy#26932. I got 10% speedup on a macro micro-benchmark that used fmpz just by inlining the type checking in __add__:
diff --git a/src/flint/types/fmpz.pyx b/src/flint/types/fmpz.pyx
index 0b16375..0a025b8 100644
--- a/src/flint/types/fmpz.pyx
+++ b/src/flint/types/fmpz.pyx
@@ -1,3 +1,5 @@
+from cpython.object cimport PyTypeObject, PyObject_TypeCheck
+
from flint.flint_base.flint_base cimport flint_scalar
from flint.utils.typecheck cimport typecheck
from flint.utils.conversion cimport chars_from_str
@@ -185,6 +187,10 @@ cdef class fmpz(flint_scalar):
return -self
def __add__(s, t):
+ if PyObject_TypeCheck(t, <PyTypeObject*>fmpz):
+ u = fmpz.__new__(fmpz)
+ fmpz_add((<fmpz>u).val, (<fmpz>s).val, (<fmpz>t).val)
+ return u
cdef fmpz_struct tval[1]
cdef int ttype = FMPZ_UNKNOWN
u = NotImplementedIt still came out notably slower than when using gmpy2 though so there is definitely some overhead in the Cython wrapper that could be reduced. In some cases gmpy2 is 2x faster for the macro benchmark.
There was a problem hiding this comment.
Maybe we should just prioritise speed for all these scalar types and accept we have to hand write all the add methods etc. seems silly to lose a bunch of performance just for a few smaller functions.
There was a problem hiding this comment.
We need to measure the effects. I think it is fine to have a framework in the superclass but then inline common cases. Note that e.g. for SymPy the case that really matters is the common case like fmpz*fmpz and to a much lesser extent fmpz*int. We could have an fmpz.__add__ method that optimises that common case as hard as possible but then just falls back on something slower that handles coercions etc:
class fmpz(scalar):
def __add__(self, other):
if PyTypeCheck(...):
# Fast optimised case
else:
# Fall back on slower generic routine
return super().__add__(self, other)It makes sense to start out having a nice generic routine in the superclass that handles everything correctly and then after doing timing measurements micro-optimise the cases that matter.
There was a problem hiding this comment.
OK. We can work this way then. I think int * type and type * type are the ones worth doing first as I think it'll be common for people to do things like 5 * type or type + 1 in their code and it would be nice for this to be fast without having to do one = type(one); type + one
There was a problem hiding this comment.
Exactly and almost always the ints would be small so the fast path could use fmpz_mul_si and friends rather than general int -> fmpz coercion.
|
I'll give this a full read later but I think it's probably good to go. |
src/flint/types/fq_default.pyx
Outdated
| - 2: `fq_default_ctx.FQ_NMOD`: Using `fq_nmod_t`, | ||
| - 3: `fq_default_ctx.FQ`: Using `fq_t`. | ||
| """ | ||
| return fq_default_ctx_type(self.val) |
There was a problem hiding this comment.
This should convert to the enum type like fq_default_type(fq_default_ctx_type(self.val)).
There was a problem hiding this comment.
Also the docstring here is missing types like NMOD and FMPZ_MOD.
Probably best to choose a single docstring that lists and explains the types (perhaps the fq_default_type docstring) and then have other places refer to it in a way that the html docs would make a clickable link like:
See :class:`~.fq_default_type` for possible types.
src/flint/types/fq_default.pyx
Outdated
| 4 : "NMOD", | ||
| 5 : "FMPZ_MOD", | ||
| } | ||
| return FQ_TYPES[self.fq_type] |
There was a problem hiding this comment.
I think it should be possible to get the string from the enum if self.fq_type returns the enum like:
return self.fq_type._name_
Then we don't need to duplicate the names and numbers here.
src/flint/types/fq_default.pyx
Outdated
| @staticmethod | ||
| def _parse_input_fq_type(fq_type): | ||
| # Allow the type to be denoted by strings or integers | ||
| FQ_TYPES = { | ||
| "FQ_ZECH" : 1, | ||
| "FQ_NMOD" : 2, |
There was a problem hiding this comment.
This dictionary is already provided by the enum type:
In [35]: fq_default_type._member_map_
Out[35]:
{'DEFAULT': <fq_default_type.DEFAULT: 0>,
'FQ_ZECH': <fq_default_type.FQ_ZECH: 1>,
'FQ_NMOD': <fq_default_type.FQ_NMOD: 2>,
'FQ': <fq_default_type.FQ: 3>,
'NMOD': <fq_default_type.NMOD: 4>,
'FMPZ_MOD': <fq_default_type.FMPZ_MOD: 5>}There was a problem hiding this comment.
I guess this function can just be:
try:
return fq_default_type._member_map_[fq_type]
except KeyError:
pass
try:
return fq_default_type._value2member_map_[fq_type]
except KeyError:
pass
raise ValueError(f"fq_type should be one of {fq_default_type._member_names_}")There was a problem hiding this comment.
I'm getting an error that fq_default_type doesn't have a member map... I'll try and figure out why
There was a problem hiding this comment.
It isn't in the docs actually. This is though:
In [75]: fq_default_type.__members__
Out[75]:
mappingproxy({'DEFAULT': <fq_default_type.DEFAULT: 0>,
'FQ_ZECH': <fq_default_type.FQ_ZECH: 1>,
'FQ_NMOD': <fq_default_type.FQ_NMOD: 2>,
'FQ': <fq_default_type.FQ: 3>,
'NMOD': <fq_default_type.NMOD: 4>,
'FMPZ_MOD': <fq_default_type.FMPZ_MOD: 5>})I guess for _value2member_map_ we need to call it:
In [76]: fq_default_type(2)
Out[76]: <fq_default_type.FQ_NMOD: 2>
Hm but something weird happens:
In [79]: fq_default_type(50)
Out[79]: <fq_default_type.FQ_NMOD|48: 50>
There was a problem hiding this comment.
I have it available in python:
In [1]: from flint import fq_default_ctx, fq_default_type
In [2]: fq_default_type
Out[2]: <flag 'fq_default_type'>
In [3]: fq_default_type._member_map_
Out[3]:
{'DEFAULT': <fq_default_type.DEFAULT: 0>,
'FQ_ZECH': <fq_default_type.FQ_ZECH: 1>,
'FQ_NMOD': <fq_default_type.FQ_NMOD: 2>,
'FQ': <fq_default_type.FQ: 3>,
'NMOD': <fq_default_type.NMOD: 4>,
'FMPZ_MOD': <fq_default_type.FMPZ_MOD: 5>}
In [4]: type(fq_default_type)
Out[4]: enum.EnumTypeBut for compilation of the cython I get:
Error compiling Cython file:
------------------------------------------------------------
...
return fq_type
@staticmethod
def _parse_input_var(var):
try:
return fq_default_type._member_map_[fq_type]
^
------------------------------------------------------------
/Users/Jack/Documents/GitHub/python-flint/src/flint/types/fq_default.pyx:65:34: _member_map_ not a known value of fq_default_type
There was a problem hiding this comment.
I guess that the C-level enum is different in the cpdef enum.
Maybe using the enum type for validation isn't quite a good fit and we just need to have a dict somewhere to map values from one to the other.
Either way we need there to be a single place where the list of values is stored rather than several different dicts spread around.
src/flint/types/fq_default.pyx
Outdated
| Finite fields can be initialized in one of two possible ways. The | ||
| first is by providing characteristic and degree: | ||
|
|
||
| >>> fq_default_ctx(5, 2, 'y', fq_type=1) | ||
| fq_default_ctx(5, 2, 'y', x^2 + 4*x + 2, 'FQ_ZECH') |
There was a problem hiding this comment.
Probably better to use fq_type='FQ_ZECH' or fq_type=fq_default_type.FQ_ZECH here rather than fq_type=1. We shouldn't encourage writing code that uses magic values like that.
src/flint/types/fq_default.pyx
Outdated
| if typecheck(obj, int): | ||
| # For small integers we can convert directly | ||
| if obj < 0 and obj.bit_length() < 31: | ||
| fq_default_set_si(fq_ele, <slong>obj, self.val) | ||
| elif obj.bit_length() < 32: | ||
| fq_default_set_ui(fq_ele, <ulong>obj, self.val) | ||
| # For larger integers we first convert to fmpz |
There was a problem hiding this comment.
The signed to unsigned cast here is unsafe:
In [65]: F = fq_default_ctx(2)
In [66]: F(-2**31+1)
---------------------------------------------------------------------------
OverflowError Traceback (most recent call last)
Cell In[66], line 1
----> 1 F(-2**31+1)
File fq_default.pyx:467, in flint.types.fq_default.fq_default_ctx.__call__()
File fq_default.pyx:492, in flint.types.fq_default.fq_default.__init__()
File fq_default.pyx:388, in flint.types.fq_default.fq_default_ctx.set_any_as_fq_default()
File fq_default.pyx:359, in flint.types.fq_default.fq_default_ctx.set_any_scalar_as_fq_default()
OverflowError: can't convert negative value to ulongI would just use a single set_si case here for both positive and negative values. Can probably catch OverflowError like:
slong cdef i
try:
i = ...
src/flint/types/fq_default.pyx
Outdated
| and self.prime() == other.prime() | ||
| and self.modulus() == other.modulus()) | ||
| else: | ||
| raise TypeError(f"Cannot compare fq_default_ctx with {type(other)}") |
There was a problem hiding this comment.
__eq__ should return NotImplemented rather than raising TypeError:
In [67]: F == None
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
Cell In[67], line 1
----> 1 F == None
File fq_default.pyx:451, in flint.types.fq_default.fq_default_ctx.__eq__()
TypeError: Cannot compare fq_default_ctx with <class 'NoneType'>
Generally considered a faux-pas for == to raise an exception rather than turning into False.
src/flint/types/fq_default.pyx
Outdated
| return f"fq_default({self.to_list(), self.ctx.__repr__()})" | ||
|
|
||
| def __hash__(self): | ||
| return hash((self.to_polynomial(), hash(self.ctx))) |
There was a problem hiding this comment.
I think we probably need coverage measurement to make sure that cases like this are checked:
In [69]: hash(F(4))
---------------------------------------------------------------------------
AttributeError Traceback (most recent call last)
Cell In[69], line 1
----> 1 hash(F(4))
File fq_default.pyx:556, in flint.types.fq_default.fq_default.__hash__()
AttributeError: 'flint.types.fq_default.fq_default' object has no attribute 'to_polynomial'
src/flint/types/fq_default.pyx
Outdated
| # For nmod we can convert by taking it as an int | ||
| # Ignores the modulus of nmod | ||
| if typecheck(obj, nmod): | ||
| fq_default_set_ui(fq_ele, <ulong>(<nmod>obj).val, self.val) | ||
| return 0 | ||
|
|
||
| # For fmpz_mod we can also convert directly | ||
| # Assumes that the modulus of the ring matches the context | ||
| if typecheck(obj, fmpz_mod): | ||
| fq_default_set_fmpz(fq_ele, (<fmpz_mod>obj).val, self.val) | ||
| return 0 |
There was a problem hiding this comment.
If this function is being used by __add__ etc then the modulus needs to be checked:
In [70]: F = fq_default_ctx(3)
In [71]: F(2) + nmod(2, 7)
Out[71]: 1There was a problem hiding this comment.
oh i thought i handled this... i must have forgotten to do it. -- oh, i did it for equality but not coercion for the addition
|
I think I have covered many of the comments above, hopefully all. but happy to re-review if needed. |
src/flint/types/fq_default.pxd
Outdated
| """ | ||
| Enum for the fq_default context types: | ||
|
|
||
| - 1. `fq_default_ctx.FQ_ZECH`: Use `fq_zech_t`, | ||
| - 2. `fq_default_ctx.FQ_NMOD`: Use `fq_nmod_t`, | ||
| - 3. `fq_default_ctx.FQ`: Use `fq_t`. | ||
| - 4. `fq_default_ctx.NMOD`: Use `nmod` for degree = 1, | ||
| - 5. `fq_default_ctx.FMPZ_MOD`: Use `fmpz_mod` for degree = 1. | ||
|
|
||
| These can be manually selected, or type: `fq_default_ctx.DEFAULT` can be used | ||
| for the implementation to be automatically decided by Flint (default), | ||
| """ | ||
| cpdef enum fq_default_type: | ||
| DEFAULT = 0 |
There was a problem hiding this comment.
Does the docstring not need to inside the enum statement?
|
One thing I realised is that we need to add this to the docs. |
|
We need a |
|
Let's get this one in now. It would be good to improve the test coverage but I suppose that can be done later. I think all the substantial issues were resolved. Thanks! |
|
Maybe we can find a hacky way to make pytest run the doctests and have coverage test this? If we get a good coverage test done im happy to circle back and improve this |
We can still use setuptools for it but I guess the setup.py just needs fixing: I think this is enough to fix it: diff --git a/setup.py b/setup.py
index d260834..82ce32d 100644
--- a/setup.py
+++ b/setup.py
@@ -103,7 +103,6 @@ ext_files = [
("flint.types.fmpz_mod_mat", ["src/flint/types/fmpz_mod_mat.pyx"]),
("flint.types.fmpq_mpoly", ["src/flint/types/fmpq_mpoly.pyx"]),
- ("flint.types.fmpz_mpoly_q", ["src/flint/types/fmpz_mpoly_q.pyx"]),
("flint.types.arf", ["src/flint/types/arf.pyx"]),
("flint.types.arb", ["src/flint/types/arb.pyx"]), |
|
I did have a look on Friday at getting Cython coverage to work with meson. The problem is that Cython's coverage plugin works by using heuristics based on setuptools. It basically needs a rewrite. Probably the rewritten version should be maintained as part of something like |
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_defaulttype which aren't implemented in cython, so we need to do nesting instead.