Add fmpz mod #83

GiacomoPope · 2023-09-14T15:42:46Z

This is a draft pull request adding the type fmpz_mod

Before continuing, I am a little confused about the fmpz_mod_ctx and it's behaving in a way which doesn't make much sense to me...

The context struct is of the form:

    ctypedef struct fmpz_mod_ctx_struct:
        fmpz_t n
        nmod_t mod
        ulong n_limbs[3]
        ulong ninv_limbs[3]
        fmpz_preinvn_struct * ninv_huge
    ctypedef fmpz_mod_ctx_struct fmpz_mod_ctx_t[1]

and as n is an fmpz_t type, I assumed this was the standard modulus. I think mod is then the modulus used for small input?

However, if I print out ctx.n and ctx.mod for various values I find:

>>> from flint import *
>>> 
>>> fmpz_mod_ctx(100)
Stuff: ([100], {'n': 100, 'ninv': 5165088340638674452, 'norm': 57}, [0, 0, 0])
>>> fmpz_mod_ctx(10**10)
Stuff: ([10000000000], {'n': 10000000000, 'ninv': 13244520931996183421, 'norm': 30}, [0, 0, 0])
>>> fmpz_mod_ctx(10**100)
Stuff: ([4611721077084439544], {'n': 0, 'ninv': 0, 'norm': 0}, [0, 0, 0])

Is it obvious here what I'm doing wrong?? Looking at the data out (a list, a dict, and a list) it seems I don't understand the "python" version of the types in this struct at all :/

oscarbenjamin · 2023-09-14T16:49:49Z

>>> fmpz_mod_ctx(100)
Stuff: ([100], {'n': 100, 'ninv': 5165088340638674452, 'norm': 57}, [0, 0, 0])
>>> fmpz_mod_ctx(10**10)
Stuff: ([10000000000], {'n': 10000000000, 'ninv': 13244520931996183421, 'norm': 30}, [0, 0, 0])
>>> fmpz_mod_ctx(10**100)
Stuff: ([4611721077084439544], {'n': 0, 'ninv': 0, 'norm': 0}, [0, 0, 0])

Here three things are being printed.

The first is self.val.n where self is the cython fmpz_mod_ctx and self.val is the C fmpz_mod_ctx_t and then self.val.n is the fmpz_t for n in the context struct. An fmpz_t is an array of 1 fmpz_struct. In the Cython code we pretend that fmpz_struct is an slong.

In other words self.val.n should be (from Cython's perspective) an array of 1 slong and so it displays as e.g. [100]. An fmpz larger than 2**62-1 will be stored as a pointer rather than an inline slong which I think is why the 3rd example shows [4611721077084439544].

The second thing printed is self.val.mod and mod is an nmod_t which is a struct having 3 values so Cython displays it as a dict showing its members.

The third thing being printed is n_limbs which is an array of 3 ulong so [0, 0, 0] makes sense.

The first two cases that you have picked are less than 64 bits (FLINT_BITS) so we go here:
https://github.com/flintlib/flint2/blob/283e514d39f8c8668123ebb981599cc94500e5c4/src/fmpz_mod/ctx_init.c#L37-L43

The third case is larger then 128 bits so we go to the general case:
https://github.com/flintlib/flint2/blob/283e514d39f8c8668123ebb981599cc94500e5c4/src/fmpz_mod/ctx_init.c#L26-L34

If you choose a modulus in between 64 and 128 bits then you should see n_limbs being used.

GiacomoPope · 2023-09-14T17:41:43Z

Thanks so much Oscar, so the "generic" thing I misunderstood was that to get nice printing for fmpz_t I need to use fmpz_get_intlong() to access the right data. Importing it from fmpz directly upset cython so for now I included it again into fmpz_mod and now the context seems to be working as intended and also prints sensible data.

>>> from flint import *
>>> 
>>> ctx = fmpz_mod_ctx(123)
>>> ctx
Context for fmpz_mod with modulus: 123
>>> 
>>> n = 2**100 - 1
>>> n
1267650600228229401496703205375
>>> ctx.set_modulus(n)
>>> ctx
Context for fmpz_mod with modulus: 1267650600228229401496703205375
>>> 
>>> n = 2**1024 - 1
>>> n
179769313486231590772930519078902473361797697894230657273430081157732675805500963132708477322407536021120113879871393357658789768814416622492847430639474124377767893424865485276302219601246094119453082952085005768838150682342462881473913110540827237163350510684586298239947245938479716304835356329624224137215
>>> ctx.set_modulus(n)
>>> ctx
Context for fmpz_mod with modulus: 179769313486231590772930519078902473361797697894230657273430081157732675805500963132708477322407536021120113879871393357658789768814416622492847430639474124377767893424865485276302219601246094119453082952085005768838150682342462881473913110540827237163350510684586298239947245938479716304835356329624224137215

I have weird things happening with the initialisation of the actual values but I'll figure this out I hope.

>>> from flint import *
>>> 
>>> mod_ctx = fmpz_mod_ctx(11)
>>> for i in range(11):
...     fmpz_mod(i, mod_ctx)
... 
fmpz_mod(0, 11)
fmpz_mod(1, 11)
fmpz_mod(2, 11)
fmpz_mod(3, 11)
fmpz_mod(4, 11)
fmpz_mod(0, 11)
fmpz_mod(1, 11)
fmpz_mod(2, 11)
fmpz_mod(3, 11)
fmpz_mod(4, 11)
fmpz_mod(0, 11)

oscarbenjamin · 2023-09-14T18:24:42Z

If fmpz_mod stores a reference to the modulus is it safe to have set_modulus modify the context?

Maybe the contexts should be immutable.

oscarbenjamin · 2023-09-14T18:25:55Z

Maybe also there should be a way to create and fnpz_mod from the context like ctx(3) or ctx.new(3)

GiacomoPope · 2023-09-14T18:27:23Z

OK very basic initialisation works, feels like that's most of the battle!

>>> from flint import *
>>> mod_ctx = fmpz_mod_ctx(163)
>>> 
>>> fmpz_mod(-1, mod_ctx)
fmpz_mod(162, 163)
>>> fmpz_mod(165, mod_ctx)
fmpz_mod(2, 163)

GiacomoPope · 2023-09-14T18:28:24Z

If fmpz_mod stores a reference to the modulus is it safe to have set_modulus modify the context?
Maybe the contexts should be immutable.

Hmm good point, this is worth thinking about. I only included it as I saw the function. I should also do the __dealloc__

GiacomoPope · 2023-09-14T18:31:28Z

Also, rather than ctx.new(val) we could have it callable so we can do ctx(val), not sure what you think fits better for the library?

Then we'd basically have ZmodN = fmpz_mod_ctx(N) and we can make elements from ZmodN(val)

oscarbenjamin · 2023-09-14T20:35:26Z

Also, rather than ctx.new(val) we could have it callable so we can do ctx(val), not sure what you think fits better for the library?

I think either is fine. You could use this like:

F11 = fmpz_mod_ctx(11)

a = F11(2)
b = F11(7)
c = a / b

GiacomoPope · 2023-09-14T20:59:18Z

I agree with the above, I made something which allows the following:

>>> from flint import *
>>> ZmodN = fmpz_mod_ctx(163)
>>> x = ZmodN(123)
>>> type(x)
<class 'flint.types.fmpz_mod.fmpz_mod'>
>>> x
fmpz_mod(123, 163)
>>> 
>>> y = fmpz(456)
>>> ZmodN(y)
fmpz_mod(130, 163)
>>> 
>>> z = fmpz_mod(789, ZmodN)
fmpz_mod(137, 163)

oscarbenjamin · 2023-09-14T22:03:25Z

Currently we have

In [3]: print(str(flint.nmod(2, 3)))
2

In [4]: print(repr(flint.nmod(2, 3)))
2

I'm not sure that I like that this does not display the modulus. It is at least inconsistent with what you show above though:

>>> x
fmpz_mod(123, 163)

This is how the SymPy equivalent looks:

In [5]: from sympy import GF

In [6]: F3 = GF(3)

In [7]: print(str(F3(2)))
2 mod 3

In [8]: print(repr(F3(2)))
SymmetricModularIntegerMod3(2)

I like the 2 mod 3 but I don't know if it is really necessary to show that (I am less keen in SymmetricModularIntegerMod3(2) but perhaps with a shorter name it could be okay).

We also have:

In [9]: flint.nmod_poly([1, 2, 3], 7)
Out[9]: 3*x^2 + 2*x + 1

In [10]: flint.nmod_mat([[1, 2], [3, 4]], 7)
Out[10]: 
[1, 2]
[3, 4]

So the modulus is not shown there. This is how those look with SymPy:

In [12]: Matrix([[1, 2], [3, 4]]).to_DM(domain=F3).to_dense()
Out[12]: DomainMatrix([[1 mod 3, 2 mod 3], [0 mod 3, 1 mod 3]], (2, 2), GF(3))

In [13]: Poly([1, 2, 3, 4], x, domain=F3)
Out[13]: Poly(x**3 - x**2 + 1, x, modulus=3)

I'm not sure what is best here. My natural inclination is that the modulus should not be invisible because nonzero characteristic is very different from zero characteristic.

We also have:

In [15]: print(str(gmpy2.mpz(3)))
3

In [16]: print(repr(gmpy2.mpz(3)))
mpz(3)

In [17]: print(str(flint.fmpz(3)))
3

In [18]: print(repr(flint.fmpz(3)))
3

GiacomoPope · 2023-09-14T22:32:19Z

I agree that nmod and fmpz_mod should match.

For another point of reference, in SageMath we have:

sage: F = GF(163)
sage: x = F.random_element()
sage: x
116
sage: str(x)
'116'
sage: 
sage: K = Zmod(163)
sage: x = K.random_element()
sage: x
136
sage: str(x)
'136'

GiacomoPope · 2023-09-14T22:34:11Z

Addition and negation and comparison are done, but not in a clean way. More to do, but will leave it for tomorrow. I have some Dad chores to do while everyone is sleeping!!

>>> from flint import *
>>> 
>>> F163 = fmpz_mod_ctx(163)
>>> assert F163.modulus() == 163
>>> 
>>> assert F163(162) == F163(-1) == -F163(1)
>>> assert F163(123) + F163(456) == F163(123 + 456) 
>>> 
>>> assert F163(162) != F163(1)

src/flint/test/test.py

GiacomoPope · 2023-09-14T23:22:16Z

Allowed the addition of:

    assert F163(123) + F163(456) == F163(123 + 456)
    assert F163(123) + F163(456) == F163(456) + F163(123)
    assert F163(123) + 456 == F163(123 + 456)
    assert F163(123) + fmpz(456) == F163(456) + F163(123)

So anything which can be cast to fmpz can now be added to fmpz_mod as well as fmpz_mod providing that the moduli match

oscarbenjamin · 2023-09-15T18:49:10Z

I think this looks good.

What is needed now is documentation. We need an fmpz_mod.rst and some docstrings to fill it out:
https://fredrikj.net/python-flint/#reference

oscarbenjamin · 2023-09-15T18:49:27Z

Looks like nmod could do with a few more docstrings...

GiacomoPope · 2023-09-15T18:57:40Z

Do you recommend one of the classes for docstring examples? Otherwise I'll just do what seems sensible.

GiacomoPope · 2023-09-15T19:03:56Z

There aren't many non-inbuilt functions so the docstrings may be a bit sparse for this type.

oscarbenjamin · 2023-09-15T21:51:03Z

A reference is needed here:

python-flint/doc/source/index.rst

Lines 41 to 50 in 11bb708

    
           .. toctree:: 
        
              :maxdepth: 1 
        
              fmpz.rst 
        
              fmpq.rst 
        
              nmod.rst 
        
              arb.rst 
        
              acb.rst 
        
              dirichlet.rst

oscarbenjamin · 2023-09-15T21:53:11Z

There aren't many non-inbuilt functions so the docstrings may be a bit sparse for this type.

Agreed but the class level docstring should show the basic idea of how to construct an fmpz_mod at least and explain what it is. An fmpz_mod is not a complicated thing but the simple definition should at least be explained because the name fmpz_mod is not exactly intuitive.

oscarbenjamin · 2023-09-15T21:57:16Z

doc/source/fmpz_mod.rst

+**fmpz** -- integers mod n
+===============================================================================
+
+.. autoclass :: flint.fmpz_mod


The fmpz_mod_ctx class also needs to be documented here.

oscarbenjamin · 2023-09-15T22:00:29Z

doc/source/fmpz_mod.rst

@@ -0,0 +1,8 @@
+**fmpz** -- integers mod n
+===============================================================================


This should say fmpz_mod.

Thank you!!

oscarbenjamin · 2023-09-15T22:01:52Z

src/flint/types/fmpz_mod.pyx

+    """
+    The *fmpz_mod* type represents integer modulo an 
+    arbitrary-size modulus. For wordsize modulus, see
+    *nmod*.


A Sphinx reference like

:class:`flint.types.nmod.nmod`

creates a clickable link in the html docs which is better.

Ahh thanks, I haven't ever used sphinx. I'll add this now.

Actually this is a shorter version:

:class:`~.nmod`

oscarbenjamin · 2023-09-15T22:06:09Z

src/flint/types/fmpz_mod.pyx

+
+    def inverse(self, check=True):
+        """
+        Computes a^-1 mod N


It should be possible to use LaTeX here but possibly the LaTeX Sphinx extension is needed but not enabled...

Yeah I'm trying to do that but the naive

math:`a^{-1} \mod N`

isn't working at least...

Oh, I got it.

GiacomoPope · 2023-09-15T22:23:14Z

Added the latex to the docstring which works (at least locally) and in the process spotted that dirichlet wasn't being imported to flint, so I fixed this! I guess dirichlet_group and dirichlet_char have no coverage in the tests? Maybe this should be an issue?

oscarbenjamin · 2023-09-15T22:25:22Z

I guess dirichlet_group and dirichlet_char have no coverage in the tests? Maybe this should be an issue?

I guess it should be. I'm not sure what the situation was before. Possibly the recent mega-refactoring affected this...

In any case if they are there then they should be made available and documented.

GiacomoPope · 2023-09-15T22:28:08Z

In any case if they are there then they should be made available and documented.

The functions are there and are fairly well docstring'd so, they're being tested in other ways. My commit above makes them available from flint again, so it's fixed the import issue and docstring / testing can always be improved so I guess there's no reason to make a specific issue right now.

GiacomoPope · 2023-09-15T22:29:00Z

I'm off for the evening now. Thanks so much for your help with this class. I'm hoping there's a magnitude less discussion for my next type as I now have a much better idea of what I should be doing and how :)

❤️

oscarbenjamin · 2023-09-15T22:29:19Z

src/flint/types/fmpz_mod.pyx

+                raise NotImplementedError
+        fmpz_mod_set_fmpz(self.val, (<fmpz>val).val, self.ctx.val)
+
+    def any_as_fmpz_mod(self, obj):


Can this be cdef?

Or otherwise a leading underscore like _any_as...

currently it shows in the docs but it should not be there.

Made it a cdef

oscarbenjamin · 2023-09-15T22:32:25Z

Unless you have anything to add I think this looks good now. More improvements can be made to the documentation but I think that is an already general problem rather than something that must be fixed in this PR.

GiacomoPope · 2023-09-15T22:34:53Z

I think this is good to go now, at least, it doesn't feel much worse than the existing types?

I want to eventually add the discrete log functions, but there's no rush for this and I'm more interested in getting fmpz_mod_poly working, as I need this for fq (and then hopefully with fq, I get fq_poly and finally I'll have enough to use python-flint for some of my isogeny things!! haha

oscarbenjamin · 2023-09-15T22:39:45Z

One thing that would be good to think about going forwards is how to share testing code a bit e.g. most of the tests for fmpz_mod here could have been to some extent shared with nmod. As we add more types the benefits of sharing test code will grow and the current structure that is like test_nmod, test_fmpz_mod etc will become less and less efficient.

We should make new test functions that are like test_add or something that test that all types can add. Then it is easy to just include a new type in the test matrix rather than needing to rewrite all tests for each type.

oscarbenjamin · 2023-09-15T22:41:14Z

I think this is good to go now

I think so too. It will be in once the tests have passed.

GiacomoPope · 2023-09-15T22:43:02Z

Agree about the testing. I also think it would be good to have randomised testing in certain places (addition, subtraction etc are all good candidates for this)

GiacomoPope added 5 commits September 14, 2023 16:37

Add new type to package

5a84520

Include additional struct

9fbe93a

Tidy up

12a87ea

Level 0 functions for fmpz_mod

b7b3df3

Start working on context for fmpz_mod

548107a

Small progress

7e7dc87

clean up context

59cfb05

Got initalisation somewhat working

0ecf9a0

Add ability to create type from context

bd09992

GiacomoPope added 2 commits September 14, 2023 23:29

Add fmpz_mod tests

06f00d1

Add comparisons and start arithmetic

9f01093

oscarbenjamin reviewed Sep 14, 2023

View reviewed changes

src/flint/test/test.py Outdated Show resolved Hide resolved

GiacomoPope added 3 commits September 15, 2023 00:12

Modify comparison tests following feedback

6f58268

allow other types in addition

f32fdfa

update tests

79db695

GiacomoPope added 2 commits September 15, 2023 00:23

add test to ensure correct error for mismatched moduli

717acf4

First draft of fmpz_mod

7a8f7e5

Add doctests and rst for fmpz_mod

42d44ad

oscarbenjamin reviewed Sep 15, 2023

View reviewed changes

GiacomoPope added 2 commits September 15, 2023 23:05

fix doc issues

1e7332f

add sphinx links to docstrings

5e09ff1

oscarbenjamin reviewed Sep 15, 2023

View reviewed changes

GiacomoPope added 3 commits September 15, 2023 23:21

Add missing export

db0c86b

Fix latex in docstring

b2dfc43

Remove unneeded docstring

ea30b64

oscarbenjamin reviewed Sep 15, 2023

View reviewed changes

Make function cdef

5dead0c

oscarbenjamin merged commit 7394b1d into flintlib:master Sep 15, 2023
16 checks passed

oscarbenjamin mentioned this pull request Sep 16, 2023

Bump version to 0.5.0 #84

Merged

oscarbenjamin mentioned this pull request Nov 15, 2023

Add fmpz_mod_mat #106

Merged

		@@ -0,0 +1,8 @@
		fmpz -- integers mod n
		===============================================================================

Add fmpz mod #83

Add fmpz mod #83

Conversation

GiacomoPope commented Sep 14, 2023 • edited Loading

oscarbenjamin commented Sep 14, 2023

GiacomoPope commented Sep 14, 2023 • edited Loading

oscarbenjamin commented Sep 14, 2023

oscarbenjamin commented Sep 14, 2023

GiacomoPope commented Sep 14, 2023

GiacomoPope commented Sep 14, 2023

GiacomoPope commented Sep 14, 2023

oscarbenjamin commented Sep 14, 2023

GiacomoPope commented Sep 14, 2023

oscarbenjamin commented Sep 14, 2023

GiacomoPope commented Sep 14, 2023

GiacomoPope commented Sep 14, 2023

GiacomoPope commented Sep 14, 2023

oscarbenjamin commented Sep 15, 2023

oscarbenjamin commented Sep 15, 2023

GiacomoPope commented Sep 15, 2023

GiacomoPope commented Sep 15, 2023

oscarbenjamin commented Sep 15, 2023

oscarbenjamin commented Sep 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GiacomoPope commented Sep 15, 2023 • edited Loading

oscarbenjamin commented Sep 15, 2023

GiacomoPope commented Sep 15, 2023

GiacomoPope commented Sep 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oscarbenjamin commented Sep 15, 2023

GiacomoPope commented Sep 15, 2023

oscarbenjamin commented Sep 15, 2023

oscarbenjamin commented Sep 15, 2023

GiacomoPope commented Sep 15, 2023

GiacomoPope commented Sep 14, 2023 •

edited

Loading

GiacomoPope commented Sep 14, 2023 •

edited

Loading

GiacomoPope commented Sep 15, 2023 •

edited

Loading