Add option to use pari for small_roots

[grhkm21]

Use pari.zncoppersmith as an alternative Coppersmith implementation for small_roots.

I decided to make pari the default because in my testings (in many practical cases, not really "edge cases") it outperforms Sage's default ("basic") implementation, both in speed and strength - see the added test for example.

[grhkm21]

Hmm, it seems that the pari zncoppersmith errors quite often for some reason (stack overflow) - I believe they keep trying to incrementing the constructed matrix size until the theoretical limit. Maybe changing the default to pari isn't a good idea? Any opinions?

[user202729]

Actually this would be a breaking change: previously small_roots(algorithm="pari") would still work, the algorithm= is just passed directly to .LLL() which it accepts.

   AVAILABLE ALGORITHMS:

   * "'NTL:LLL'" -- NTL's LLL + choice of "fp"

   * "'fpLLL:heuristic'" -- fpLLL's heuristic + choice of "fp"

   * "'fpLLL:fast'" -- fpLLL's fast + choice of "fp"

   * "'fpLLL:proved'" -- fpLLL's proved + choice of "fp"

   * "'fpLLL:wrapper'" -- fpLLL's automatic choice (default)

   * "'pari'" -- pari's qflll

Maybe rename the new algorithm= to small_roots_algorithm=, the old algorithm= to lll_algorithm=, and deprecate the old algorithm=?

that is, if the zncoppersmith() is still faster than the old LLL(algorithm=pari).

---

Another alternative I can think of is algorithm="pari_zncoppersmith being treated as a special case.

Worst case (if default to pari isn't better in all cases) you can try to do something such as (pseudocode)

t = Timer(5, lambda: warning("looks like small_roots() is taking a long time, try using algorithm='pari'?")
B = B.LLL()
t.cancel()

then the user still get notification to possibly change, but only if it's slow.

[github-actions]

Documentation preview for this PR (built with commit 0197b8f; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

[grhkm21]

I'll fix the algorithm keyword first. I think it'll be a bad idea to have lll_algorithm, we should just keep the old algorithm, but indeed rename the current algorithm to small_roots_algorithm.

Also, as the example suggests, pari is not only faster than Sage, but also stronger, but it errors sometimes. I don't think there's an objective "better", but maybe we should catch the PariError and tell the user to try Sage instead (though 99.999% Sage will just return [])

[user202729]

If sage is likely to return [] anyway it seems more reasonable to just set the default to pari then.

[grhkm21]

@user202729 this is ready for review :)

[user202729]

what is # time random supposed to do? (I guess it's equivalent to # long time, random? If that's so consider changing it to that)

And why exactly is it random? (isn't the root unique and it's sort of guaranteed pari can find it anyway? Of course the flip side is sage's algorithm may happen to improve in the future for some reason, but in that case at least remove the # random in pari case to actually test that pari can find the root)

[grhkm21]

@user202729 I think the time the test took was random and might time out randomly, but I can't replicate that behaviour now 😓 I will just remove the tag entirely. The output is not random anyways, since I set the seed above.

[user202729]

looks good to me.

I wonder if

The implementation of this algorithm follows Alexander May’s PhD thesis referenced below.

is still true. (not that it matters much.)

the timeout might as well be one of the random timeout issues that happen once in a while (which as far as I can tell nobody knows why)

[grhkm21]

Oh, is there a timeout in any CI? I don't see any errors 🤔

[user202729]

I mean things like #37066 or #39183 . So far it hasn't failed in that specific file, but it might have just been an instance of the same thing.