Mod poly and mpn_mod random generation - #2448
Conversation
I think you need to rebase on main and force-push to get a clean diff. |
c1be23c to
ba6af38
Compare
| } | ||
| } | ||
|
|
||
| void fmpz_randm_nonzero(fmpz_t f, flint_rand_t state, const fmpz_t m) { |
There was a problem hiding this comment.
The current behaviour does not have uniform distribution (it is more likely to generate 1 than other values). Maybe generate uniformly in [0,m-2] (being careful with m=1 or such corner cases), and then add 1?
There was a problem hiding this comment.
Does m stands for uniform? For me, it stands for modulus since the element is upper bounded by a modulus and is not just a random element with a set number of bits. Sorry, I did not meant to make something uniform.
The functions fmpz_mod_poly_randtest used randm. I prefered creating a randm_nonzero function than using randtest_nonzero.
There was a problem hiding this comment.
What I meant is that currently, fmpz_randm generates a uniform integer in [0, m) (all values have equal probability), as can be seen from the code. The fact that the distribution is uniform is not explicitly indicated in the documentation, but that's ok, it is the "canonical distribution" for a finite set.
So if a fmpz_randm_nonzero function is added, it seems to me that it should also have uniform distribution (choosing uniformly in [1, m)), unless this is tricky to make. In this case it is not tricky: apply fmpz_randm with m-1 to get a uniform choice in [0, m-1), and then add 1, this gives you a uniform choice in [1, m).
| fmpz_mod_poly_fit_length(poly, len, ctx); | ||
| _fmpz_vec_zero(poly->coeffs, len); | ||
| fmpz_randm(poly->coeffs + 0, state, fmpz_mod_ctx_modulus(ctx)); | ||
| fmpz_randm_nonzero(poly->coeffs + 0, state, fmpz_mod_ctx_modulus(ctx)); |
There was a problem hiding this comment.
Why this change, or why using nonzero almost everywhere?
There was a problem hiding this comment.
To have at least a randfull-like functionality. See my comments above.
There was a problem hiding this comment.
As suggested in other functions, I think that in these randtest functions we may as well rely on the existing fmpz_randtest_mod.
|
I cancelled the CI workflow because they were still in testing after almost 4 hours. Please review the code carefully to see what changes to existing functions could have led to this, thanks! (I pointed at some modifications in my comments, but there may be others) |
|
There is probably an infinite loop, I'll do a manual bisect. |
The randtest functions are using `randm` which generates positive coefficients in [0, m) range with m the modulus given by the context. The documentation told that the coefficients are randomly signed, so I fixed that. I introduced a `randm_nonzero` function in fmpz to ensure that coefficients are nonzero and replaced all occurences of `randm` in `fmpz_mod_poly_randtest` functions by this new function. Apart from the `rand` function, this introduces a `rand_monic` and a `rand_irreducible` function.
I added _fmpz_mod_vec_rand with the intent to use it in _mpn_mod_vec_rand but did not use it in the end. This reverts commit f98e52c.
21ee353 to
59f5c7d
Compare
|
The tests halt indefinitely after |
|
Ok, I guess that if a polynomial has nonzero coefficients, it is irreducible with much less probability. This could slow down the tests. I should indeed give up on a "randfull-like" functionality for fmpz_mod_poly_randtest functions but then what would be the point of making another |
| .. function:: void nmod_poly_randtest(nmod_poly_t poly, flint_rand_t state, slong len) | ||
|
|
||
| Generates a random polynomial with length up to ``len``. | ||
| Generates a random and sparse with increased probability polynomial with length up to ``len``. |
There was a problem hiding this comment.
| Generates a random and sparse with increased probability polynomial with length up to ``len``. | |
| Generates a random polynomial with length up to ``len``, with for each coefficient the probability of some special values increased (see :func:`n_randtest`). This function is intended for use in test code. |
There was a problem hiding this comment.
This is sparse with increased proba, but also special values are more likely than with a uniform distribution. So maybe,
| Generates a random and sparse with increased probability polynomial with length up to ``len``. | |
| Generates a random polynomial with length up to ``len``, sparse with increased probability, and with coefficients generated by :func:`n_randtest` . |
| } | ||
| } | ||
|
|
||
| void fmpz_randm_nonzero(fmpz_t f, flint_rand_t state, const fmpz_t m) { |
There was a problem hiding this comment.
What I meant is that currently, fmpz_randm generates a uniform integer in [0, m) (all values have equal probability), as can be seen from the code. The fact that the distribution is uniform is not explicitly indicated in the documentation, but that's ok, it is the "canonical distribution" for a finite set.
So if a fmpz_randm_nonzero function is added, it seems to me that it should also have uniform distribution (choosing uniformly in [1, m)), unless this is tricky to make. In this case it is not tricky: apply fmpz_randm with m-1 to get a uniform choice in [0, m-1), and then add 1, this gives you a uniform choice in [1, m).
| { | ||
| if (len == 0) | ||
| { | ||
| flint_throw(FLINT_ERROR, "Exception (fmpz_mod_poly_randtest_irreducible). len == 0.\n"); |
There was a problem hiding this comment.
| flint_throw(FLINT_ERROR, "Exception (fmpz_mod_poly_randtest_irreducible). len == 0.\n"); | |
| flint_throw(FLINT_ERROR, "Exception (fmpz_mod_poly_rand_irreducible). len == 0.\n"); |
There was a problem hiding this comment.
There is a mismatch function name / name in exception.
There was a problem hiding this comment.
(modified comment after checking the diff more carefully)
| fmpz_mod_poly_fit_length(poly, len, ctx); | ||
| _fmpz_vec_zero(poly->coeffs, len); | ||
| fmpz_randm(poly->coeffs + 0, state, fmpz_mod_ctx_modulus(ctx)); | ||
| fmpz_randm_nonzero(poly->coeffs + 0, state, fmpz_mod_ctx_modulus(ctx)); |
There was a problem hiding this comment.
As suggested in other functions, I think that in these randtest functions we may as well rely on the existing fmpz_randtest_mod.
This is likely indeed. I haven't looked closely at the test files and the one that is failing, but let's suppose that some test includes working modulo 2, then the new randtest using
In my last batch of comments, you can see that I suggest using |
f5b0816 to
eafe4cf
Compare
|
I changed randm_non_zero t to randtest calls. I gave one extra bit for the random generation before taking the modulus. If you think it is not necessary, I will remove it. |
|
Hello, sorry for not making quickly the last modifications needed to close this PR.
Is this still actual? I can't remember if I did this change. I'll look into it tomorrow. Pay attention to the previous commit. I remove an invocation to fmpz_bits. It apparently fixes a segfault, but I am not sure why it was invocated in the first place. Could you check out for this during review? Otherwise, I made two changes:
|
| Sets `f` to a random polynomial with up to the given length and where | ||
| each coefficient has up to the given number of bits. The coefficients | ||
| are uniformly generated random numbers in `[0, n)`, where `n` is the modulus given by the context `ctx`. |
There was a problem hiding this comment.
I would suggest to copy-paste the doc of e.g. nmod_poly:
| Sets `f` to a random polynomial with up to the given length and where | |
| each coefficient has up to the given number of bits. The coefficients | |
| are uniformly generated random numbers in `[0, n)`, where `n` is the modulus given by the context `ctx`. | |
| Generates a random polynomial with length up to `len`, with uniformly chosen coefficients. |
| Sets `f` to a random monic polynomial with up to the given length and where | ||
| each coefficient has up to the given number of bits. The coefficients | ||
| are uniformly generated random numbers in `[0, n)`, where `n` is the modulus given by the context `ctx`. |
There was a problem hiding this comment.
I would suggest to copy-paste the doc of e.g. nmod_poly:
| Sets `f` to a random monic polynomial with up to the given length and where | |
| each coefficient has up to the given number of bits. The coefficients | |
| are uniformly generated random numbers in `[0, n)`, where `n` is the modulus given by the context `ctx`. | |
| Generates a random monic polynomial with length `len`, with uniformly chosen coefficients. |
| Sets `f` to a random irreducible polynomial with up to the given length and where each coefficient has up to the given number of bits. The coefficients | ||
| are uniformly generated random numbers in `[0, n)`, where `n` is the modulus given by the context `ctx`. |
There was a problem hiding this comment.
As above, I would suggest something more concise (and there is nothing about number of bits here):
| Sets `f` to a random irreducible polynomial with up to the given length and where each coefficient has up to the given number of bits. The coefficients | |
| are uniformly generated random numbers in `[0, n)`, where `n` is the modulus given by the context `ctx`. | |
| Generates a random irreducible polynomial with up to the given length and where the coefficients | |
| are generated uniformly. |
| Sets `f` to a random polynomial with up to the given length and where | ||
| each coefficient has up to the given number of bits. The coefficients | ||
| are signed randomly. | ||
| are random numbers in `[1, n)`. |
There was a problem hiding this comment.
Perhaps a small mistake here, I don't think this function has been changed, and the previous doc was correct it seems(?).
|
|
||
| .. function:: void nmod_poly_rand(nmod_poly_t poly, flint_rand_t state, slong len) | ||
|
|
||
| Generates a random polynomial with length up to ``len``. |
There was a problem hiding this comment.
| Generates a random polynomial with length up to ``len``. | |
| Generates a random polynomial with length up to ``len`` and coefficients generated uniformly. |
|
|
||
| .. function:: void nmod_poly_rand_monic(nmod_poly_t poly, flint_rand_t state, slong len) | ||
|
|
||
| Generates a random monic polynomial with length up to ``len``. |
There was a problem hiding this comment.
| Generates a random monic polynomial with length up to ``len``. | |
| Generates a random monic polynomial of length ``len`` and non-leading coefficients generated uniformly. |
|
|
||
| .. function:: void nmod_poly_rand_irreducible(nmod_poly_t poly, flint_rand_t state, slong len) | ||
|
|
||
| Generates a random irreducible polynomial with length up to ``len``. |
There was a problem hiding this comment.
| Generates a random irreducible polynomial with length up to ``len``. | |
| Generates a random irreducible polynomial with length up to ``len`` with coefficients generated uniformly. |
| .. function:: void nmod_poly_randtest(nmod_poly_t poly, flint_rand_t state, slong len) | ||
|
|
||
| Generates a random polynomial with length up to ``len``. | ||
| Generates a random and sparse with increased probability polynomial with length up to ``len``. |
There was a problem hiding this comment.
This is sparse with increased proba, but also special values are more likely than with a uniform distribution. So maybe,
| Generates a random and sparse with increased probability polynomial with length up to ``len``. | |
| Generates a random polynomial with length up to ``len``, sparse with increased probability, and with coefficients generated by :func:`n_randtest` . |
| .. function:: void nmod_poly_randtest_monic(nmod_poly_t poly, flint_rand_t state, slong len) | ||
|
|
||
| Generates a random monic polynomial with length ``len``. | ||
| Generates a random and sparse with increased probability monic polynomial with length ``len``. |
There was a problem hiding this comment.
See the comment above on a similar function.
|
Here is a first batch of comments, on the documentation. |
| .. function:: void fmpz_randm_nonzero(fmpz_t f, flint_rand_t state, const fmpz_t m) | ||
|
|
||
| Generates a random integer in the range `1` to `m - 1` inclusive. Requires | ||
| `m \geq 2`, otherwise an exception will result. |
There was a problem hiding this comment.
| `m \geq 2`, otherwise an exception will result. | |
| `m \geq 2`, otherwise an exception is thrown. |
| { | ||
| if (fmpz_cmp_ui(m, 2) <= 0) | ||
| { | ||
| flint_throw(FLINT_ERROR, "Exception (fmpz_randm_nonzero). m <= 2.\n"); |
There was a problem hiding this comment.
| flint_throw(FLINT_ERROR, "Exception (fmpz_randm_nonzero). m <= 2.\n"); | |
| flint_throw(FLINT_ERROR, "Exception (fmpz_randm_nonzero). m < 2.\n"); |
| void | ||
| fmpz_randm_nonzero(fmpz_t f, flint_rand_t state, const fmpz_t m) | ||
| { | ||
| if (fmpz_cmp_ui(m, 2) <= 0) |
There was a problem hiding this comment.
| if (fmpz_cmp_ui(m, 2) <= 0) | |
| if (fmpz_cmp_ui(m, 2) < 0) |
because the issue is when m is strictly less than 2
| fmpz_randm(f, state, mMinusOne); // 0..m-2 | ||
| fmpz_add_ui(f, f, 1); // 1..m-1 |
There was a problem hiding this comment.
| fmpz_randm(f, state, mMinusOne); // 0..m-2 | |
| fmpz_add_ui(f, f, 1); // 1..m-1 | |
| fmpz_randm(f, state, mMinusOne); /* 0..m-2 */ | |
| fmpz_add_ui(f, f, 1); /* 1..m-1 */ |
| } | ||
|
|
||
| void | ||
| fmpz_randm_nonzero(fmpz_t f, flint_rand_t state, const fmpz_t m) |
There was a problem hiding this comment.
| fmpz_randm_nonzero(fmpz_t f, flint_rand_t state, const fmpz_t m) | |
| fmpz_randm_not_zero(fmpz_t f, flint_rand_t state, const fmpz_t m) |
(to follow the naming used elsewhere: fmpq_randtest_not_zero, arf_randtest_not_zero, fq_default_rand_not_zero, etc)
|
|
||
| Generates a random integer in the range `0` to `m - 1` inclusive. | ||
|
|
||
| .. function:: void fmpz_randm_nonzero(fmpz_t f, flint_rand_t state, const fmpz_t m) |
There was a problem hiding this comment.
| .. function:: void fmpz_randm_nonzero(fmpz_t f, flint_rand_t state, const fmpz_t m) | |
| .. function:: void fmpz_randm_not_zero(fmpz_t f, flint_rand_t state, const fmpz_t m) |
| fmpz_randtest_unsigned(f->coeffs + i, state, fmpz_bits(fmpz_mod_ctx_modulus(ctx))); | ||
| fmpz_mod(f->coeffs + i, f->coeffs + i, fmpz_mod_ctx_modulus(ctx)); |
There was a problem hiding this comment.
For these two lines (and all the similar ones below in this file), I would suggest to make use of fmpz_randtest_mod
| { | ||
| if (len == 0) | ||
| { | ||
| flint_throw(FLINT_ERROR, "Exception (fmpz_mod_poly_randtest_irreducible). len == 0.\n"); |
There was a problem hiding this comment.
There is a mismatch function name / name in exception.
| fmpz_randtest_unsigned(poly->coeffs + random_idx + 1, state, fmpz_bits(fmpz_mod_ctx_modulus(ctx))); | ||
| fmpz_mod(poly->coeffs + random_idx + 1, poly->coeffs + random_idx + 1, fmpz_mod_ctx_modulus(ctx)); |
There was a problem hiding this comment.
See above, about using fmpz_randtest_mod
| #include "nmod_poly.h" | ||
| #include "nmod_poly_factor.h" | ||
|
|
||
| // Rand functions -> random dense polynomials with high probability |
There was a problem hiding this comment.
| // Rand functions -> random dense polynomials with high probability | |
| /* rand functions -> coefficients generated uniformly */ |
Yes, your change is fine, and I agree this was an extraneous invocation that was probably added by mistake. |
Follow-up of #2430 .
Adds a
randfunction:It seems that implementations of nmod_mpoly
randtestfunctions generate uniformly random coefficients.I did not add
randfunctions for basic types like fmpz_mod nor nmod.I plan to add a
randfunction formpn_mod_polyandmpn_mod_mat.Please forgive me, I based my branch on my previously non-merged branch and it needs some cleanup.