search

3-manifolds / SnapPy

SnapPy is a package for studying the topology and geometry of 3-manifolds, with a focus on hyperbolic structures. It is based on the SnapPea kernel written by Jeff Weeks.
https://snappy.computop.org/
89 stars 41 forks source link

Replace imports from sage.all #123

Open mkoeppe opened 1 month ago

mkoeppe commented 1 month ago

Preparation for using modularized distributions of the Sage library.

Done in part using sage -fiximports.

unhyperbolic commented 1 month ago

Thanks for looking into this.

How stable are these import paths? How many versions of sage do they go back?

The CI is configured to run SnapPy against various docker containers with different SageMath versions. It reports that 4 out of 11 suites failed. Unfortunately, github doesn't let me see which :(

In the past, we had large try: ... except ImportError: ... blocks to account for different sage versions.

mkoeppe commented 1 month ago

How stable are these import paths? How many versions of sage do they go back?

AFAICS these imports are all unchanged in Sage over at least the past 5 years

The CI is configured to run SnapPy against various docker containers with different SageMath versions. It reports that 4 out of 11 suites failed. Unfortunately, github doesn't let me see which :(

Are you saying that the current branch was already tested?

By the way, my motivation here is to get SnapPy to run on top of my modularized fork of Sage https://github.com/passagemath/passagemath

NathanDunfield commented 1 month ago

Our CI tests SnapPy on the official Sage Docker images, from 9.3 (circa 2021) up to 10.4. We do not test against the development branch of Sage.

I enabled the CI for this PR and tweaked the CI config. The PR fails when running the doctests for Sage 9. and passes for Sage 10.. The common error is:

  File "/home/sage/sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/snappy/snap/polished_reps.py", line 18, in <module>
    from sage.combinat.subset import powerset
ImportError: cannot import name 'powerset' from 'sage.combinat.subset' (/home/sage/sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage/combinat/subset.py)
mkoeppe commented 1 month ago

Thanks @NathanDunfield. Would changes like just pushed in ada49ea5097b7f9d1cef80b0ddb9d72aeec52255 be acceptable?

NathanDunfield commented 1 month ago

Would changes like just pushed in ada49ea be acceptable?

Overall, it looks good to me. For the frequently imported things, it might be better to do the import in sage_helper.py, especially if it needs a try-except block. See the example of ComplexField in that file.

mkoeppe commented 1 month ago

Here's a more complete version (still has a failure in doctesting snappy.database that I don't understand at the moment). With this version, I can get snappy.test to run on top of only passagemath-flint, passagemath-repl, passagemath-groups, passagemath-symbolics, passagemath-plot, passagemath-graphs.