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

Work-around that CyPari2 has removed prec_words_to_dec #124

Open
NathanDunfield opened this issue Nov 4, 2024 · 17 comments
Open

Work-around that CyPari2 has removed prec_words_to_dec #124

NathanDunfield opened this issue Nov 4, 2024 · 17 comments

Comments

@NathanDunfield
Copy link
Member

With the newest version 2.2.0 of cypari2 importing snappy results in the below error. It seems that CyPari2 has removed prec_words_to_dec. SnapPy uses that function to estimate how many digits of the decimal representation of a volume are correct. Only when running in Sage does SnapPy use CyPari2 (otherwise it uses CyPari). This only affects people using SnapPy in the not-yet released Sage 10.5.

Traceback (most recent call last):
 File "/usr/bin/SnapPy", line 33, in <module>
   sys.exit(load_entry_point('snappy==3.1.1', 'console_scripts',
'SnapPy')())
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File
"/usr/bin/SnapPy", line 25, in importlib_load_entry_point return
next(matches).load() ^^^^^^^^^^^^^^^^^^^^
 File "/usr/lib/python3.12/importlib/metadata/__init__.py", line 205,
in load module = import_module(match.group('module'))
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 File "/usr/lib/python3.12/importlib/__init__.py", line 90, in
import_module return _bootstrap._gcd_import(name[level:], package,
level) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 File "<frozen importlib._bootstrap>", line 1387, in _gcd_import
 File "<frozen importlib._bootstrap>", line 1360, in _find_and_load
 File "<frozen importlib._bootstrap>", line 1310, in
_find_and_load_unlocked File "<frozen importlib._bootstrap>", line 488,
in _call_with_frames_removed File "<frozen importlib._bootstrap>", line
1387, in _gcd_import File "<frozen importlib._bootstrap>", line 1360,
in _find_and_load File "<frozen importlib._bootstrap>", line 1331, in
_find_and_load_unlocked File "<frozen importlib._bootstrap>", line 935,
in _load_unlocked File "<frozen importlib._bootstrap_external>", line
995, in exec_module File "<frozen importlib._bootstrap>", line 488, in
_call_with_frames_removed File
"/usr/lib/python3.12/site-packages/snappy-3.1.1-py3.12-linux-x86_64.egg/snappy/__init__.py",
line 6, in <module> from .SnapPy import (AbelianGroup, File
"cython/numbers/double.pyx", line 1, in init SnapPy File
"/usr/lib/python3.12/site-packages/snappy-3.1.1-py3.12-linux-x86_64.egg/snappy/number.py",
line 6, in <module> from .pari import * File
"/usr/lib/python3.12/site-packages/snappy-3.1.1-py3.12-linux-x86_64.egg/snappy/pari.py",
line 17, in <module> from cypari2.pari_instance import
(prec_words_to_dec, ImportError: cannot import name 'prec_words_to_dec'
from 'cypari2.pari_instance'
(/usr/lib/python3.12/site-packages/cypari2/[pari_instance.cpython-312-x86_64-linux-gnu.so](http://pari_instance.cpython-312-x86_64-linux-gnu.so/)).
Did you mean: 'prec_bits_to_dec'?
@culler
Copy link
Member

culler commented Nov 4, 2024

Maybe this problem is going to vanish.

With Sage 10.5beta8 I am seeing no errors when importing the precision converters:

sage: from cypari2.pari_instance import prec_words_to_dec
sage: from cypari2.pari_instance import prec_words_to_bits
sage: from cypari2.pari_instance import prec_bits_to_dec
sage: from cypari2.pari_instance import prec_dec_to_bits

I think that may mean that those functions have been restored in CyPari2, presumably for compatibility reasons.

@NathanDunfield
Copy link
Member Author

What version of cypari2 does 10.5beta8 come with? I'm not seeing any substantial commits to cypari2 since they released cypari 2.2.0. Could it have to do with the underlying version of Pari itself?

@culler
Copy link
Member

culler commented Nov 4, 2024

It comes with CyPari 2.2.0 and Pari 2.15.4. There has been a lot of activity at CyPari2 related to precision issues with Pari 2.17. So I would guess that it does indeed have to do with the version of Pari.

@culler
Copy link
Member

culler commented Nov 4, 2024

Maybe the workaround, for now, is to use CyPari2 from Sage rather than installing the bleeding edge on top of Sage's version.

@NathanDunfield
Copy link
Member Author

I tried the official cypari==2.2.0 wheels from PyPi (Python 3.11, linux) and they to have prec_words_to_dec. But those are based on Pari 2.15.4 as well.

@NathanDunfield
Copy link
Member Author

The report came from a user of the Arch Linux distro, which does rolling releases and currently has Pari 2.17.

@culler
Copy link
Member

culler commented Nov 10, 2024

I looked at the cypari2 github site, and as far as I can tell, they have not removed prec_words_to_dec. They did make several changes to accommodate pari 2.17, but those changes did not include removing that function from the pari_instance module. On the other hand, this page indicates that the Arch linux pari package is using Pari 2.17.

I suspect that when Arch switched to pari 2.17 the maintainer of the Arch Sage package was forced to do his own adaptation of cypari2 to pari 2.17, and he chose to delete pari_words_to_dec and thereby break SnapPy in Arch sage 10.4.

So it is not clear to me what we should do. We could modify SnapPy to avoid using prec_words_to_dec now. That would accommodate a random choice made by the Arch maintainer. But after cypari2 does its next release, and after sage incorporates that release into sage, the current SnapPy code will work again. So the change is not necessary, except for the purpose of making snappy work on one release of sage for one linux distro which happened to make an incompatible change to the cypari2 package.

I think that may mean that we should do nothing.

@NathanDunfield
Copy link
Member Author

@culler Unfortunately, I believe this cypari2 PR to support Pari 2.17 will remove prec_words_to_dec and will be merged shortly. Let's see if they are amendable to keeping it.

@NathanDunfield
Copy link
Member Author

They are open to keeping prec_words_to_dec and friends, at least for Pari 2.15, but it's not clear whether they would work for Pari 2.17. It looks like we use these three times in submodules of verify, a few times in number, and also in snap.shapes. Thoughts? Also, tagging @unhyperbolic as the author of verify.

@culler
Copy link
Member

culler commented Nov 11, 2024 via email

@tornaria
Copy link

Maybe self.gen.precision() solves your issue (get the real precision in decimals) and it is well supported by pari, and should work regardless of internals.

There's also self.gen.bitprecision() and can also use nbits2prec() and prec2nbits() to convert between precision in bits and pari precision as in the parameter prec of pari functions. All well supported and will do the right thing for each version of pari.

@culler
Copy link
Member

culler commented Jan 7, 2025

I think this is fixed in my latest commit. To test, I used %pip to install cypari2-2.2.1 in SageMath-10.5. Then I installed SnapPy from the current tip of our repo in SageMath-10.5. Then this is what I see:

% sage
┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 10.5, Release Date: 2024-12-04                    │
│ Using Python 3.12.5. Type "help()" for help.                       │
└────────────────────────────────────────────────────────────────────┘
sage: import cypari2
sage: cypari2.prec_words_to_dec
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
Cell In[2], line 1
----> 1 cypari2.prec_words_to_dec

AttributeError: module 'cypari2' has no attribute 'prec_words_to_dec'
sage: import snappy
sage: M = snappy.Manifold('m125')
sage: v = M.volume(bits_prec=1024)
sage: snappy.number.Number(v).accuracy
308

@tornaria
Copy link

tornaria commented Jan 7, 2025

Please see my comment 7f0bac4#r150980881

@tornaria
Copy link

tornaria commented Jan 7, 2025

Also, I think for a little while there was a cypari 2.2.1 in pypi with this function removed, but the function was restored (with a deprecated warning).

@NathanDunfield
Copy link
Member Author

Also, I think for a little while there was a cypari 2.2.1 in pypi with this function removed, but the function was restored (with a deprecated warning).

Here "this function" refers to prec_bits_to_dec?

@tornaria
Copy link

tornaria commented Jan 7, 2025

Also, I think for a little while there was a cypari 2.2.1 in pypi with this function removed, but the function was restored (with a deprecated warning).

Here "this function" refers to prec_bits_to_dec?

I meant prec_words_to_dec. The functions to convert bits to dec were never removed (they are just multiplication by lg(10) or so)

@NathanDunfield
Copy link
Member Author

I checked that the commit aae6a46 works on Arch Linux, which already has PARI 2.17 and no prec_words_to_dec function. Closing the ticket.

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

No branches or pull requests

3 participants