Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow comparison with nmod to fmpz and fmpz_mod #178

Closed
wants to merge 1 commit into from

Conversation

GiacomoPope
Copy link
Contributor

Following some discussion in #97 this modifies comparison in nmod to allow nmod(a, b) == fmpz(a % b) to be true as well as nmod(a, b) == fmpz_mod_ctx(b)(a) to be true.

@oscarbenjamin
Copy link
Collaborator

@fredrik-johansson do you have any views on this?

I assume that the original code that does this was written by you:

In [4]: nmod(2, 3) == fmpz(2)
Out[4]: False

In [5]: nmod(2, 3) == 2
Out[5]: True

I don't know to what extent that was an intentional decision or just something that was left unresolved for the future.

@oscarbenjamin
Copy link
Collaborator

We also have:

In [8]: flint.nmod(1, 3) * flint.fmpz(2)
Out[8]: 2

In [9]: type(flint.nmod(1, 3) * flint.fmpz(2))
Out[9]: flint.types.nmod.nmod

I think that if fmpz coerces to nmod then they should be able to compare equal.

@oscarbenjamin
Copy link
Collaborator

And now I notice this one:

In [12]: flint.fmpz_mod_ctx(13)(1) * flint.nmod(1, 13)
---------------------------------------------------------------------------
TypeError

@GiacomoPope
Copy link
Contributor Author

Yeah I think I didn't implement coercion either way here because I wasn't sure how to pick between the two.

IMO it's ok to say the two values are equal but not be able to multiple them together but this could easily be configured differently.

@GiacomoPope
Copy link
Contributor Author

GiacomoPope commented Aug 8, 2024

although maybe you could say that if the modulus fits in a u32 and nmod could be used, it should be used as fmpz_mod will just be slower? so fmpz_mod * nmod should return an nmod

Another option is that nmod and fmpz_mod should be totally distinct and incomparable and unable to be coerced between each other. I really can't tell how these two should behave. Of course, once we have a parent like Zmod which selects nmod and fmpz_mod automatically it'll be easier

@oscarbenjamin
Copy link
Collaborator

Of course, once we have a parent like Zmod which selects nmod and fmpz_mod automatically it'll be easier

Yes, in retrospect I'm not sure what benefit there is in having them as distinct types in python-flint. There are good reasons for nmod and fmpz_mod to be distinct at the C level because the memory management is very different and doing anything with fmpz_mod potentially requires heap allocations. At the CPython level it's all on the heap so we might as well just have Zmod and have it choose which type to use internally with no noticeable difference to the user (much like fq_default).

Leaving them not interoperable also seems fine though for now. There should be some way to perform explicit coercions though:

In [8]: ctx = flint.fmpz_mod_ctx(13)

In [9]: ctx(flint.nmod(1, 13))
---------------------------------------------------------------------------
NotImplementedError

@GiacomoPope
Copy link
Contributor Author

I'm happy to write (tomorrow) a small addition to fmpz_mod and nmod to allow conversions between each other.

Another PR could do the same between nmod_poly and fmpz_mod_poly potentially

@oscarbenjamin
Copy link
Collaborator

I'm happy to write (tomorrow) a small addition to fmpz_mod and nmod to allow conversions between each other.

We need something systematic for explicit conversions. I think that means that the first thing to fix is having a context for all types including nmod, fmpz and fmpq. Changing nmod to use a context is the biggest change and also the most useful.

I started working on a context for nmod but it became clear that I also needed to change all the other nmod_* types and I didn't finish after that. I'll push it up as a PR soon...

@oscarbenjamin
Copy link
Collaborator

I'll push it up as a PR soon...

I've just pushed gh-179.

@GiacomoPope GiacomoPope deleted the nmod_equality branch August 12, 2024 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants