Adapt to precision changes in pari 2.17

[antonio-rojas]

Since 2.17 pari stores the precision in bits instead of words. Port cypari2 to work with it:

• Rename prec_bits_to_words to prec_bits_to_pari and outsource it to upstream's nbits2prec, so it works with all pari versions
• Remove unused and no longer useful prec_words_to_bits, prec_dec_to_words and prec_words_to_dec functions
• Use upstream's LOWDEFAULTPREC as default precision

[dimpase]

there are doctest failures like this.

https://github.com/sagemath/cypari2/actions/runs/11186104097/job/31100414929#step:5:322

in cypari2.pari_instance.__test__.default_bitprec (line 367)
Failed example:
    default_bitprec()
Differences (ndiff with -expected +actual):
    - 64
    + 3

it's probably hard to avoid differentiating between pari versions up to 2.15 and 2.17+ in this test.

[antonio-rojas]

it's probably hard to avoid differentiating between pari versions up to 2.15 and 2.17+ in this test.

That was actally an issue with my code, this test should always return 64.

[antonio-rojas]

Tests seem fine now. However, I'm seeing a weird issue that I don't understand. With this patch, in sage I get

sage: N = 1715761513; E = EllipticCurve(Integers(N), [3,-13]); P = E(2,1); LCM([2..60])*P
[...]
PariError: impossible inverse in Fp_inv: Mod(26927, 1715761513)

which shows that the PariError is not being intercepted in https://github.com/sagemath/sage/blob/10.5.beta6/src/sage/schemes/elliptic_curves/ell_point.py#L3915. If I change except PariError to except Exception it works, and querying the exception type shows that it is indeed a PariError.

The issue doesn't happen without this patch (but of course lots of precision related stuff is broken). Any idea what's going on here? Why should these changes affect PariError handling?

[videlec]

Tests seem fine now. However, I'm seeing a weird issue that I don't understand. With this patch, in sage I get

sage: N = 1715761513; E = EllipticCurve(Integers(N), [3,-13]); P = E(2,1); LCM([2..60])*P
[...]
PariError: impossible inverse in Fp_inv: Mod(26927, 1715761513)

which shows that the PariError is not being intercepted in https://github.com/sagemath/sage/blob/10.5.beta6/src/sage/schemes/elliptic_curves/ell_point.py#L3915. If I change except PariError to except Exception it works, and querying the exception type shows that it is indeed a PariError.

The issue doesn't happen without this patch (but of course lots of precision related stuff is broken). Any idea what's going on here? Why should these changes affect PariError handling?

Could it be a PariError from an other cypari2 (sounds super weird)? Did you check in the catch whether PariError == PariError?

[antonio-rojas]

Ah, found the issue. sage/schemes/elliptic_curves/ell_point.py has

try:
    from sage.libs.pari.all import pari, PariError
    from cypari2.pari_instance import prec_words_to_bits
except ImportError:
    PariError = ()

which of course fails to properly import PariError after prec_words_to_bits removal.

[tornaria]

Maybe replace this by DEFAULTPREC where it is used (prec_bits_to_pari only?) and get rid of the global variable?

[tornaria]

Ah, found the issue. sage/schemes/elliptic_curves/ell_point.py has

try:
    from sage.libs.pari.all import pari, PariError
    from cypari2.pari_instance import prec_words_to_bits
except ImportError:
    PariError = ()

which of course fails to properly import PariError after prec_words_to_bits removal.

But in fact, prec_words_to_bits is only used once in sagemath, as follows:

    while prec_words_to_bits(log_pari.precision()) < precision:

Maybe it makes more sense to replace this by

    while log_pari.bitprecision() < precision:

(as a matter of fact: log_pari.precision() is the precision in decimals so this seems to be a bug)

I am not sure if prec_words_to_bits() and prec_bits_to_words() should be kept or not for backwards compatibility.

But I would rather not introduce the new functions prec_pari_to_bits() and prec_bits_to_pari() which are just a wrapper around prec2nbits() and nbits2prec(). Neither one is useful for backwards compatibility (sagemath has to be patched anyway, so we may as well patch it not to need these wrappers).

[antonio-rojas]

@antonio-rojas did you see antonio-rojas#1?

No, I don't get notifications for PR's. Merged now, thanks.

Also, what do you think about replacing prec_bits_to_pari straight by nbits2prec? I think this can work out if we replace the default precision=0 by precision=64 or precision=BITS_IN_LONG or just precision=DEFAULT_BITPRECISION.

That's what I've been trying, but couldn't make it work. Testing your patch now.

[tornaria]

@antonio-rojas did you see antonio-rojas#1?

No, I don't get notifications for PR's. Merged now, thanks.

Thank you!

Also, what do you think about replacing prec_bits_to_pari straight by nbits2prec? I think this can work out if we replace the default precision=0 by precision=64 or precision=BITS_IN_LONG or just precision=DEFAULT_BITPRECISION.

That's what I've been trying, but couldn't make it work. Testing your patch now.

BTW, you rewrote default_bitprec() using LOWDEFAULTPREC but this is effectively equivalent to a bit precision of BITS_IN_LONG (i.e. 64 in 64-bit arches and 32 in 32-bit arches). However, in the current implementation default_bitprec() is always 64. This corresponds to pari DEFAULTPREC (and this is what I called DEFAULT_BITPREC which in effect it should always be 64, but it seems nice to use it with a descriptive name)

I think keeping the default bit precision at 64 regardless of architecture is the best choice, so that doctesting is consistent.

[antonio-rojas]

@antonio-rojas did you see antonio-rojas#1?

That's what I've been trying, but couldn't make it work. Testing your patch now.

Everything works fine from both cypari and sage sides. Want to submit a PR for proper attribution?

[tornaria]

@antonio-rojas did you see antonio-rojas#1?

That's what I've been trying, but couldn't make it work. Testing your patch now.

Everything works fine from both cypari and sage sides. Want to submit a PR for proper attribution?

Done.

I think this is looking good, I will be testing on 32 bits soon (maybe also with pari 2.15).

I wonder if, given that sagemath 10.4 uses prec_words_to_bits() we should be keeping it for a while (deprecated). Otherwise, upgrading cypari2 will break sagemath 10.4. Moreover, it seems important that we push the 2-line change to remove the (incorrect) use of prec_words_to_bits() in sagemath 10.5 (even if we don't get support for pari 2.17, we make it easier to move forward).

[tornaria]

Indeed, sagemath 10.4 breaks with this. I made a PR to restore prec_words_to_bits() as a deprecated function: antonio-rojas#3

[tornaria]

This looks good to me. How do we move forward?

It would be nice to have this merged with a 2.3.0 release in view? Maybe useful to prerelease a 2.3.0b1 for wider testing?

[NathanDunfield]

@tornaria @antonio-rojas Unfortunately, this PR will break SnapPy inside Sage because the we use prec_dec_to_words and prec_words_to_dec. This has already been reported to us in the wild by a user of Arch which I believe uses and early version of this PR. Is there any chance that prec_dec_to_words and prec_words_to_dec could just be deprecated and preserved for backwards compatibility as was done with prec_words_to_bits? That would be very helpful to @culler and myself.

[tornaria]

@tornaria @antonio-rojas Unfortunately, this PR will break SnapPy inside Sage because the we use prec_dec_to_words and prec_words_to_dec. This has already been reported to us in the wild by a user of Arch which I believe uses and early version of this PR. Is there any chance that prec_dec_to_words and prec_words_to_dec could just be deprecated and preserved for backwards compatibility as was done with prec_words_to_bits? That would be very helpful to @culler and myself.

Note that this PR is not even merged, much less released.

I don't think there's any problem with keeping (with deprecation) all the functions that were removed: prec_bits_to_words, prec_dec_to_words and prec_words_to_dec. I already readded prec_words_to_bits which is used by sagemath up to 10.4.

Bear in mind that, depending on how you use these functions, readding them will not necessarily make your program work correctly with pari 2.17. Readding will make sure that the next version of cypari2 still works with old sagemath and snappy when using pari 2.15.

Maybe these functions, in addition to raise a deprecation warning, should also return an error when used with pari 2.17?

In any case, you should consider replacing the usage of these functions in snappy, otherwise it will be very difficult for you to support pari versions 2.15 and 2.17 with the same codebase. Pari macros nbits2prec and prec2nbits may be useful.

[NathanDunfield]

Note that this PR is not even merged, much less released.

The author of this PR, @antonio-rojas, is also the maintainer of cypari2 on Arch Linux, which ships with Pari 2.17. The current Arch patch overlaps with this PR and in particular removes prec_dec_to_words and prec_words_to_dec.

I don't think there's any problem with keeping (with deprecation) all the functions that were removed: prec_bits_to_words, prec_dec_to_words and prec_words_to_dec. I already readded prec_words_to_bits which is used by sagemath up to 10.4.

Great!

Bear in mind that, depending on how you use these functions, readding them will not necessarily make your program work correctly with pari 2.17. Readding will make sure that the next version of cypari2 still works with old sagemath and snappy when using pari 2.15.

Noted, we'll have to look into this more.

[antonio-rojas]

We need this in and a new release made so we can unblock the pari 2.17 upgrade in sage. Anything missing?

[tornaria]

Edit: moved this comment to sagemath/sage#38749 (comment)

[tornaria]

We need this in and a new release made so we can unblock the pari 2.17 upgrade in sage. Anything missing?

Not a blocker, maybe nice to have #165 (comment)

However, it's unclear the scope, should we do just struct forprime_t or maybe a few more obvious ones. The immediate goal of forprime_t is so that we can avoid messing with internals of pari prime iteration.

[dimpase]

Is this ready to be merged?

[antonio-rojas]

Is this ready to be merged?

I think so, yes. There's the question of the prec_dec_to_words restoration, but once this is released pari will be upgraded to 2.17 in sagemath, so all code using prec_dec_to_words will be broken anyway and will have to be ported. In any case, I'm not planning to work on this myself.

[tornaria]

Is this ready to be merged?

I think so, although the question remains about whether to remove or not some functions that are no longer used in sagemath 10.5, but might be used somewhere else (snappy)

We could keep those functions but they only make sense with pari 2.15. See my comment #166 (comment)

Maybe @NathanDunfield can tell us if they updated snappy to not use those.

[culler]

SnapPy is still using prec_dec_to_words. We plan to change that for the next release, but even after that not all users will have the newest release. It would be very helpful to us (i.e. to our users) if that- function could be left in place even though Sage is not using it. Thanks. - Marc

…

On Mon, Jan 6, 2025 at 2:12 PM Gonzalo Tornaría ***@***.***> wrote: Is this ready to be merged? I think so, although the question remains about whether to remove or not some functions that are no longer used in sagemath 10.5, but might be used somewhere else (snappy) We could keep those functions but they only make sense with pari 2.15. See my comment #166 (comment) <#166 (comment)> Maybe @NathanDunfield <https://github.com/NathanDunfield> can tell us if they updated snappy to not use those. — Reply to this email directly, view it on GitHub <#166 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJ6CPZHMI76ECOK6X4FNSD2JLPSHAVCNFSM6AAAAABPMQEZSGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNZTHA2DCOJTGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[tornaria]

Is this ready to be merged?

I think so, yes. There's the question of the prec_dec_to_words restoration, but once this is released pari will be upgraded to 2.17 in sagemath, so all code using prec_dec_to_words will be broken anyway and will have to be ported. In any case, I'm not planning to work on this myself.

I agree this is a waste of energy. My vote would be to move all in to pari 2.17, both in cypari and in sagemath. Distros and software that need pari 2.15 can stay with cypari 2.2.0 and sagemath 10.5.

However, keeping prec_words_to_dec seems not a big deal: we already kept prec_words_to_bits, and those two are the only functions used in snappy afaict.

In any case, it should be very easy to replace those as we did in sagemath, for instance:

• replace prec_words_to_bits(gen.sizeword()) by gen.bitprecision()
• replace prec_words_to_dec(gen.sizeword()) by gen.precision()

[tornaria]

SnapPy is still using prec_dec_to_words.

Is it prec_dec_to_words or prec_words_to_dec that you use ?

We plan to change that for the next release,

Just do the change, it's very simple. This issue 3-manifolds/SnapPy#124 was opened 2 months ago.

but even after that not all users will have the newest release. It would be very helpful to us (i.e. to our users) if that- function could be left in place even though Sage is not using it.

The reason to remove it is not that sagemath is not using it. But that the prec_*_to_words is possibly being misused: if one uses the output of prec_*_to_words(...) for the prec parameter in pari functions, then ones code will be broken with pari 2.17. Removing the function forces one to fix the code, this is a good thing. The compatible way (which would work with both pari 2.15 and 2.17) is to use the pari functions nbits2prec and prec2nbits.

The prec_words_to_* functions, otoh, seem less dangerous so I would agree keeping them for a few months, if you agree to stop using them asap (it should be a trivial one line for one line change).