flintlib
Correct FQ_DEFAULT_TYPE comparison in evaluation
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
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.
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.
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.
Thanks for the commit!
A couple of comments:
- Could you stylize the comments as
/* comment */? - Could you randomize even more? More specifically, could you randomize the extension degree? Randomization of the type is also nice, but not necessary.
- 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
\otimesis addition or multiplication?
The actual test was copied from an exiting one:
https://github.com/flintlib/flint/blob/05340d2a7349761aff2cd3370810c93e2174ce46/src/fmpz_mod_poly/test/t-evaluate_fmpz.c#L43-L46
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?
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?
The actual test was copied from an exiting one:
https://github.com/flintlib/flint/blob/05340d2a7349761aff2cd3370810c93e2174ce46/src/fmpz_mod_poly/test/t-evaluate_fmpz.c#L43-L46
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.
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.
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?
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.
/* 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?
My bad. Yes, looks good! Will wait until Fredrik comes back from his vacation to see if he has any more opinions.
Should be fine to merge after streamlining the test code on top of #2050.
Sorry for the delay in this, the new test is cleaner and uses the random context method from my other PR