-
Notifications
You must be signed in to change notification settings - Fork 41
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
Replace imports from sage.all #123
base: master
Are you sure you want to change the base?
Conversation
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. |
AFAICS these imports are all unchanged in Sage over at least the past 5 years
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 |
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:
|
Thanks @NathanDunfield. Would changes like just pushed in ada49ea be acceptable? |
…r imports from Sage
Overall, it looks good to me. For the frequently imported things, it might be better to do the import in |
… instead of using try..except for imports
Here's a more complete version (still has a failure in doctesting |
Preparation for using modularized distributions of the Sage library.
Done in part using
sage -fiximports
.