Added fmpz_mod_mpoly_q module #2327

andrii302 · 2025-05-29T17:14:25Z

In this fork I have written a module for rational polynomials on the mod some prime number field F. The functions and tests were created as an extension of fmpz_mpoly_q module but for the finite field F.

fredrik-johansson · 2025-05-29T20:33:48Z

src/gr.h

@@ -698,7 +698,7 @@ void gr_method_tab_init(gr_funcptr * methods, gr_method_tab_input * tab);
 typedef enum
 {
    GR_CTX_FMPZ, GR_CTX_FMPQ, GR_CTX_FMPZI,
-    GR_CTX_FMPZ_MOD, GR_CTX_NMOD, GR_CTX_NMOD8, GR_CTX_NMOD32, GR_CTX_MPN_MOD,
+    GR_CTX_FMPZ_MOD, GR_CTX_FMPZ_MOD_POLY, GR_FMPZ_MOD_MPOLY, GR_CTX_FMPZ_MOD_MPOLY_Q, GR_CTX_NMOD, GR_CTX_NMOD8, GR_CTX_NMOD32, GR_CTX_MPN_MOD,


These should go further down, by the other polynomial types.

fredrik-johansson · 2025-05-29T20:34:22Z

src/fmpz_mod_mpoly_q/is_canonical.c

+    if (fmpz_mod_mpoly_is_zero(fmpz_mod_mpoly_q_denref(res), ctx))
+        return 0;
+
+    if (fmpz_sgn(fmpz_mod_mpoly_q_denref(res)->coeffs) < 0)


This test is redundant. The test for monic denominator below suffices.

fredrik-johansson · 2025-05-29T20:34:54Z

src/fmpz_mod_mpoly_q/is_canonical.c

+        ans_coeff = fmpz_mod_is_one(fmpz_mod_mpoly_q_denref(res)->coeffs + 0, ctx->ffinfo);
+        fmpz_mod_mpoly_clear(g, ctx);
+
+        return ans*ans_coeff;


I'd just use && here.

fredrik-johansson · 2025-05-29T20:38:33Z

src/fmpz_mod_mpoly_q.h

+/* Polynomial helper functions */
+
+FMPZ_MOD_MPOLY_Q_INLINE void
+_fmpz_vec_content2(fmpz_t res, const fmpz * vec, slong len, const fmpz_t inp)


This function can be removed in favor of _fmpz_vec_content_chained (yes, I realize that it is copied from fmpz_mpoly_q.h where the same applies).

Actually, I'm confused about why this function is needed at all for fmpz_mod_mpoly_q. Surely extracting integer GCDs does nothing since we are over a field and normalize the denominator to be monic in the end?

You are correct, I have just realized that I don't need gcd's for the coeff's at all. I am removing it from the code right now.

andrii302 · 2025-05-30T01:19:15Z

There's a new commit where I made all the requested changes, fixed the incorrect/missing _clear statements and got rid of any use of GCD for the coefficients.

fredrik-johansson · 2025-05-30T12:39:20Z

src/fmpz_mod_mpoly_q/inv.c

+        fmpz_mod_mpoly_neg(fmpz_mod_mpoly_q_numref(res), fmpz_mod_mpoly_q_numref(res), ctx);
+        fmpz_mod_mpoly_neg(fmpz_mod_mpoly_q_denref(res), fmpz_mod_mpoly_q_denref(res), ctx);
+    }
+    fmpz_mod_mpoly_q_canonicalise(res, ctx);


You don't need a full canonicalisation here, because you already know that the numerator and denominator are coprime. Just need to make the denominator monic.

fredrik-johansson · 2025-05-30T12:41:14Z

src/fmpz_mod_mpoly_q/inv.c

+
+    fmpz_mod_mpoly_swap(fmpz_mod_mpoly_q_numref(res), fmpz_mod_mpoly_q_denref(res), ctx);
+
+    if (fmpz_sgn(fmpz_mod_mpoly_q_denref(res)->coeffs) < 0)


So this should be if (!fmpz_is_one(fmpz_mod_mpoly_q_denref(res)->coeffs)) ... make monic.

fredrik-johansson · 2025-05-30T12:43:22Z

src/fmpz_mod_mpoly_q/print_pretty.c

+    {
+        fmpz_mod_mpoly_print_pretty(fmpz_mod_mpoly_q_numref(f), x, ctx);
+    }
+    else if (fmpz_mod_mpoly_is_fmpz(fmpz_mod_mpoly_q_denref(f), ctx))


I don't think this branch can be reached. If the denominator is a constant, it should be 1.

fredrik-johansson · 2025-05-30T12:48:18Z

There are many branches throughout the code where you check fmpz_mod_mpoly_is_fmpz(den, ...). But in all such cases den will necessarily be 1 for canonicalised inputs, so you can just check fmpz_mod_mpoly_is_one instead and then simplify the calculations.

andrii302 · 2025-06-02T00:00:59Z

The latest commit deals with the commented parts + added tests for gr_fmpz_mod_mpoly_q module. The functions were also simplified on one part, and made sure to output the canonical form quolynomial on another.

fredrik-johansson · 2025-06-02T21:33:24Z

src/gr/fmpz_mod_mpoly_q.c

+    {GR_METHOD_CTX_IS_COMMUTATIVE_RING,         (gr_funcptr) gr_generic_ctx_predicate_true},
+    {GR_METHOD_CTX_IS_INTEGRAL_DOMAIN,          (gr_funcptr) gr_generic_ctx_predicate_true},
+    {GR_METHOD_CTX_IS_UNIQUE_FACTORIZATION_DOMAIN,    (gr_funcptr) gr_generic_ctx_predicate_true},
+    {GR_METHOD_CTX_IS_FIELD,                    (gr_funcptr) gr_generic_ctx_predicate_false},


is_field should be true

fredrik-johansson · 2025-06-02T21:34:04Z

src/gr/fmpz_mod_mpoly_q.c

+    {GR_METHOD_CTX_IS_UNIQUE_FACTORIZATION_DOMAIN,    (gr_funcptr) gr_generic_ctx_predicate_true},
+    {GR_METHOD_CTX_IS_FIELD,                    (gr_funcptr) gr_generic_ctx_predicate_false},
+    {GR_METHOD_CTX_IS_FINITE,                   (gr_funcptr) gr_generic_ctx_predicate_false},
+    {GR_METHOD_CTX_IS_FINITE_CHARACTERISTIC,    (gr_funcptr) gr_generic_ctx_predicate_false},


is_finite_characteristic should be true

fredrik-johansson · 2025-06-02T21:48:04Z

This looks quite good now. Just some minor things to fix:

Correct the is_field and is_finite_characteristic predicates.
Add your name to the copyright headers. No need to do this for files where you only made a small edit to existing code, but at least do it for fmpz_mod_mpoly_q.h and fmpz_mod_mpoly_q/*.c.
Document gr_ctx_init_fmpz_mod_mpoly_q in gr_domains.rst.
I suggest adding a comment both in fmpz_mod_mpoly_q.rst and in the documentation line for gr_ctx_init_fmpz_mod_mpoly_q to the effect "The user is responsible for verifying that m is prime. Composite m will lead to undefined behavior."

Once these are fixed, I will do one more careful reading of the code before merging.

fredrik-johansson · 2025-06-02T22:11:03Z

I'm a bit concerned by the fact that quite a few lines in the arithmetic operations don't have test coverage according to codecov. It should be close to 100% (compare https://app.codecov.io/gh/flintlib/flint/tree/main/src%2Ffmpz_mpoly_q). Either there are still some redundant branches, or the randtest function needs to be improved, or the test code needs modifying in some way.

fredrik-johansson · 2025-06-03T08:22:13Z

In add and sub for example you have the conditional

    if (fmpz_mod_is_one(y_den->coeffs, ctx->ffinfo))
    {

which will always be true since y is in canonical form.

fredrik-johansson · 2025-06-03T08:24:17Z

In set_fmpq, add_fmpq etc. you need error handling for the case where the modulus divides the denominator.

andrii302 · 2025-06-05T20:47:28Z

The required fixes to the code and documentation were made. The only parts in add and sub unreachable by tests that were left, were for the case when y_den==x_den and GCD(res_num, x_den)!=1, which is mathematically possible, but simply too improbable for the tests. In general the coverage must be significantly better now.

andrii302 and others added 10 commits May 22, 2025 18:23

Added init and clear

705c435

Continue adding methods to gr

7bd5bbf

Merge branch 'flintlib:main' into main

7f8e60d

Added add and neg

84aa73e

Major bug fixes

09a3966

Merge branch 'flintlib:main' into main

9ed5e35

Added some tests

8c4ccea

Finished and passed the tests

b29457a

Documentation added

d6c18e4

removed unfinished example

4fab147

fredrik-johansson reviewed May 29, 2025

View reviewed changes

andrii302 added 2 commits May 30, 2025 03:02

Removed gdc's for coeffs. added fixes

937c2e4

Minor changes

4ef0ea1

fredrik-johansson reviewed May 30, 2025

View reviewed changes

Added gr tests and optimised the functions

160289a

andrii302 and others added 2 commits June 2, 2025 02:01

Merge branch 'flintlib:main' into main

c84441b

Optimized tests

21c4cf9

fredrik-johansson reviewed Jun 2, 2025

View reviewed changes

andrii302 and others added 2 commits June 5, 2025 11:18

Merge branch 'flintlib:main' into main

b062a43

Improved code coverage and documentation

3022509


		fmpz_mod_mpoly_swap(fmpz_mod_mpoly_q_numref(res), fmpz_mod_mpoly_q_denref(res), ctx);

		if (fmpz_sgn(fmpz_mod_mpoly_q_denref(res)->coeffs) < 0)

Added fmpz_mod_mpoly_q module #2327

Are you sure you want to change the base?

Added fmpz_mod_mpoly_q module #2327

Conversation

andrii302 commented May 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrii302 commented May 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredrik-johansson commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrii302 commented Jun 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredrik-johansson commented Jun 2, 2025

Uh oh!

fredrik-johansson commented Jun 2, 2025

Uh oh!

fredrik-johansson commented Jun 3, 2025

Uh oh!

fredrik-johansson commented Jun 3, 2025

Uh oh!

andrii302 commented Jun 5, 2025

Uh oh!

Uh oh!

fredrik-johansson commented May 30, 2025 •

edited

Loading