Correct FQ_DEFAULT_TYPE comparison in evaluation

[GiacomoPope]

The method fq_default_poly_evaluate_fq_default() had an incorrect comparison for the default type, which lead to a segfault when using this method with type fmpz_mod.

This PR should fix the issue and hence fix #2046

[albinahlback]

Thanks, looks good! Do you agree @fredrik-johansson?

[GiacomoPope]

I don't know if I need to add tests to show that this is fixed? I've not contributed to flint before -- this was identified while working on python-flint.

[albinahlback]

Yes, it would be good with a test. Could you implement one? The tests are under fq_default_poly/test. You create a test file (in this case, t-evaluate_fq_default.c), and then include it in the main file main.c in the same directory. You can test that module with make check MOD=fq_default_poly.

[GiacomoPope]

OK -- I had a few silly bugs along the way, but I think I now have a semi-decent check for all 5 fq_default types in place.

[albinahlback]

Thanks for the commit!

A couple of comments:

1. Could you stylize the comments as /* comment */?
2. Could you randomize even more? More specifically, could you randomize the extension degree? Randomization of the type is also nice, but not necessary.
3. What are you exactly testing? Obviously it checks that it doesn't crash, but apart from that it only seems to check that two consecutive evaluations yield the same result, no? My knowledge on finite fields is not the best, but wouldn't it be possible to check that the result is consistent with general algebraic rules like $\mathrm{eval}(p_{1}) \otimes \mathrm{eval}(p_{2}) = \mathrm{eval}(p_{1} \otimes p_{2})$, where \otimes is addition or multiplication?

[GiacomoPope]

The actual test was copied from an exiting one:

flint/src/fmpz_mod_poly/test/t-evaluate_fmpz.c

Lines 43 to 46 in 05340d2

	fmpz_mod_poly_evaluate_fmpz(b, f, a, ctx);
	fmpz_mod_poly_evaluate_fmpz(a, f, a, ctx);
	result = (fmpz_equal(a, b));

I can change this is you want, I just did what already existed.

I can randomise the type, but the idea was to ensure all 5 types were always captured. If you would prefer I can use the fq_default_ctx_randtest() and simply ask for a new random context for everything single call?

[GiacomoPope]

Oh... Apparently https://flintlib.org/doc/fq_default.html#c.fq_default_ctx_randtest is in the documentation but not in the code?

Should an issue be made for this?

[albinahlback]

The actual test was copied from an exiting one:

flint/src/fmpz_mod_poly/test/t-evaluate_fmpz.c

Lines 43 to 46 in 05340d2

	fmpz_mod_poly_evaluate_fmpz(b, f, a, ctx);
	fmpz_mod_poly_evaluate_fmpz(a, f, a, ctx);
	result = (fmpz_equal(a, b));

You only copied parts of that test. That part only checks aliasing, not other mathematical consistencies.

I can randomise the type, but the idea was to ensure all 5 types were always captured.

It is better with randomization as you can change seeds etc. All types will be covered given large enough test multipliers.

If you would prefer I can use the fq_default_ctx_randtest() and simply ask for a new random context for everything single call?

Oh... Apparently https://flintlib.org/doc/fq_default.html#c.fq_default_ctx_randtest is in the documentation but not in the code?

Should an issue be made for this?

Given that fq_default_ctx_init_rand or something similar does not exist, I think the current solution is good.

[albinahlback]

Insert test multiplier here

[GiacomoPope]

I purposefully didn't because the multiplier is in the inner test function. Would you like it here too? I was worried about having this multiplier squared would be an issue

[GiacomoPope]

I added it in, feel free to ask me to remove it again

[albinahlback]

The problem here is that you are only able to capture bugs for cases on the form $3^k$. You should try to randomize this even more.

But be careful here since the fq_zech type does not like when you go big.

[GiacomoPope]

For FQ_ZECH and FQ_NMOD I have two tests likely to pick each of these but allow fq_default to automatically select between these to avoid needing to be precise about when to use one or the other. Hopefully you are ok with what i have picked

[GiacomoPope]

did you need anything else from me for this PR?

[albinahlback]

Sorry for the delay. Will respond in a couple of days.

[albinahlback]

Does it no longer check aliasing?

[GiacomoPope]

I'm not sure I understand

[albinahlback]

I'm not sure I understand

We usually check aliasing, such as fmpz_add(x, x, y) works and is consistent with fmpz_add(z, x, y) (here z and x are two different instances). When I looked at it earlier, it looked like it had been removed.

[GiacomoPope]

You want the evaluation to check aliasing then? I can do this if you want I didn't intentionally add or remove anything though. Is the current test not sufficient?

[albinahlback]

You want the evaluation to check aliasing then?

Since it was in the previous commit (and contained in similar tests), I would very much like that!

I can do this if you want I didn't intentionally add or remove anything though.

Don't worry, didn't cross my mind.

Is the current test not sufficient?

Well, it should work (since it is inherited from fq_nmod_poly etc.), but it is always good to have the test in the case that the infrastructure changes in the future.

[GiacomoPope]

        /* Evaluate the polynomials f1, f2, f3 = f1 * f2 on the element a */
        fq_default_poly_evaluate_fq_default(c, f3, a, ctx);
        fq_default_poly_evaluate_fq_default(b, f2, a, ctx);
        fq_default_poly_evaluate_fq_default(a, f1, a, ctx);

this code is currently in place, it doesn't quite check that

fq_default_poly_evaluate_fq_default(c, f1, a, ctx);
fq_default_poly_evaluate_fq_default(a, f1, a, ctx);
fq_default_equal(a, c, ctx);

But it implicitly checks this as it computes c = f3(a) and later a = f1(a) and ensures f3(a) = f1(a) * f2(a) -- would you like me to add more tests on top of what is here?

[albinahlback]

My bad. Yes, looks good! Will wait until Fredrik comes back from his vacation to see if he has any more opinions.

[fredrik-johansson]

Should be fine to merge after streamlining the test code on top of #2050.

[GiacomoPope]

Sorry for the delay in this, the new test is cleaner and uses the random context method from my other PR