-
Notifications
You must be signed in to change notification settings - Fork 25
fixing unnecessary import of ProductTree #162
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR attempts to fix a failing import by commenting out the ProductTree import from sage.rings.generic. However, ProductTree is actually used in the code on line 428 within the KummerLineIsogeny_VeluSqrt._hI_precomputation method. While the challenge code doesn't trigger this code path due to using small primes (< 587) with a default threshold of 1500, commenting out the import will break the VéluSqrt functionality and cause a NameError if that code path is ever executed.
Key Changes
- Commented out the import statement for
ProductTreefromsage.rings.generic
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Sage imports | ||
| from sage.all import prod, ZZ, PolynomialRing | ||
| from sage.rings.generic import ProductTree | ||
| #from sage.rings.generic import ProductTree |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commenting out the ProductTree import will cause a NameError when KummerLineIsogeny_VeluSqrt is used (line 428). While the challenge code uses only small primes (< 587) and the default threshold is 1500, making VeluSqrt unused in normal operation, commenting out this import breaks the VeluSqrt functionality entirely. Consider either: 1) importing ProductTree from the correct location if the path has changed in newer Sage versions, 2) implementing a local ProductTree class, or 3) removing the VeluSqrt class entirely if it's not needed.
| #from sage.rings.generic import ProductTree | |
| from sage.rings.generic import ProductTree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code branch is never used and by commenting out the import, the solution becomes possible. The platform has Sage 10.6 but this struggles with SUID. Using the Python 3.8, only Sage 9.0 from 2020 is available. As ProductTree became available around Sage 9.7 in 2022, this seems the easiest way to proceed. Removing the entire class seems much to desructive.
Line
from sage.rings.generic import ProductTreein montgomery_isogenies/kummer_isogeny.py fails but is not necessary.Fixes #144