>>> 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.

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)

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.

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

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)

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__

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)

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

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)

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

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'

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)

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

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

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

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

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

A reference is needed here:

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.

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?

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.

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.

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 :)

❤️

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.

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

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.

I think this is good to go now

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

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)