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

Introduce fq_default_ctx_init_randtest() #2050

Merged

Conversation

GiacomoPope
Copy link
Contributor

While working on #2047 I noticed that the documentation included fq_default_ctx_randtest() but the function itself was not available. This PR introduces the function fq_default_ctx_init_randtest() named to match fq_ctx_init_randtest(), fq_nmod_ctx_init_randtest() and fq_zech_ctx_init_randtest().

Originally I was going to select between these three but then I noticed fq_ctx_init_randtest() only works with nmod sized primes, so I have followed this design for the default too.

To init a random context, I rely on _nmod_poly_conway_rand() to compute a random prime / degree and then initialise the fq_default_ctx_t from these.

I have added a test to t-init.c trying to follow the behaviour of the original tests, where I simply check I can create random fq_default_t elements without a crash.

Comment on lines 107 to 113
.. function:: void fq_default_ctx_init_randtest(fq_default_ctx_t ctx, flint_rand_t state, int type)

Initializes ``ctx`` to a random finite field. Assumes that
``fq_default_ctx_init`` has not been called on ``ctx`` already.
Initializes ``ctx`` to a random finite field where the prime and degree is
set according to ``type``.
Assumes that ``fq_default_ctx_init`` has not been called on ``ctx`` already.
To see what prime and degrees may be output, see
``type`` in :func:`_nmod_poly_conway_rand`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear: You are not referring to the internal "type" for fq_default (i.e. fmpz, nmod etc.)?

I have to think about the randomness thing (i.e. which type). Now, this module is not very much tested inside FLINT, unfortunately (however it is tested in Nemo/Oscar), but in general we would like to (1) minimize the time for testing and (2) reduce the complexity of usage. A separate parameter for type obviously increases the complexity, and, furthermore, is inherited from nmod_poly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type here is the one passed into the generation of prime,degree from the nmod conway function and is not related at all the the fq_default type

The design here, hopefully, simply does what practically would happen if you asked for a random context from fq, fq_nmod and fq_zech

Comment on lines 95 to 96
fq_default_clear(fq, ctx);
fq_default_randtest(fq, state, ctx);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Order has to change here, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, sorry about that. Will fix now.

@fredrik-johansson
Copy link
Collaborator

This doesn't do what I expect. I would expect it to create a fq_default context with a randomly chosen internal representation, not necessarily the default chosen by fq_default_ctx_init.

@GiacomoPope
Copy link
Contributor Author

Thanks for the feedback, I have tried adapting the function to better match your expectation.

/* Create GF(p^d) for FQ_ZECH context */
case FQ_DEFAULT_FQ_ZECH:
fmpz_randprime(prime, state, 2 + n_randint(state, 4), 1);
deg = 1 + n_randint(state, 4);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this can generate GF(31^4) which is quite large for the Zech representation as far as basic testing goes anyway. Maybe drop both the n_randint(state, 4) to n_randint(state, 3).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have modified these two values.

@fredrik-johansson fredrik-johansson merged commit decd504 into flintlib:main Sep 5, 2024
13 checks passed
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.

3 participants