Refactor to break cyclic import in complex numbers
tobiasdiez opened this issue · 32 comments
Importing complex_mpfr does currently not work without previously importing sage.all. This is because of a couple of cyclic imports (e.g. complex_number -> complex_double -> complex_number).
In this ticket, we break these cyclic imports by narrowing down some of the global module imports to local function-wide imports. This was not possible in the case of the CCtoCDF class, since there ComplexDoubleElement is cimported, which has to be done on the module level. I've hence extracted this class to a new module complex_conversion.pyx.
With these changes, the executing the following code
from sage.rings.complex_mpfr import ComplexField
print(ComplexField())
using ./venv/bin/python3 works. Previously, it failed with
File "/home/tobias/sage/src/test.py", line 1, in <module>
from sage.rings.complex_mpfr import ComplexField
File "sage/rings/complex_mpfr.pyx", line 1, in init sage.rings.complex_mpfr
File "sage/rings/real_mpfr.pyx", line 1, in init sage.rings.real_mpfr
File "sage/libs/mpmath/utils.pyx", line 17, in init sage.libs.mpmath.utils
ImportError: cannot import name ComplexField
Depends on #32612
Depends on #32610
Depends on #32174
CC: @mkoeppe @tscrim @antonio-rojas
Component: refactoring
Author: Tobias Diez
Branch/Commit: 401fee5
Reviewer: Matthias Koeppe
Issue created by migration from https://trac.sagemath.org/ticket/30741
Changes of this type need careful review regarding possible performance penalties from the import
Instead of
+ from sage.rings.complex_conversion import CCtoCDF
+ return CCtoCDF(S, self)
Why don't you add a proper .pxd file and does a cimport at module level?
A module level cipmort CCtoCDF would lead again to a circular import: complex_double -> complex_conversion (for CCtoCDF) -> complex_double (for ComplexDoubleElement).
Replying to @tobiasdiez:
A module level
cipmort CCtoCDFwould lead again to a circular import: complex_double -> complex_conversion (for CCtoCDF) -> complex_double (for ComplexDoubleElement).
cimport does not trigger the Python module import (the things in the .pyx). It is just reading the corresponding .pxd file.
EDIT: and it is perfectly fine to have circular imports here as these are guarded with macros at the C level
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
Setting a new milestone for this ticket based on a cursory review.
With #32612 you can also use isinstance(..., sage.rings.abc.ComplexIntervalField) instead of using a local import
Branch pushed to git repo; I updated commit sha1. New commits:
dfa1de0 | Use sage.rings.abc.ComplexIntervalField instead of soon-deprecated class |
Dependencies: #32612
still needs work or ready for review?
Branch pushed to git repo; I updated commit sha1. New commits:
fb09fda | Fix up imports |
I've fixed the branch but does it actually solve anything?
Description changed:
---
+++
@@ -1,3 +1,20 @@
-Importing `complex_number` does currently not work without previously importing `sage.all`. This is because of a couple of cyclic imports (e.g. `complex_number` -> `complex_double` -> `complex_number`).
+Importing `complex_mpfr` does currently not work without previously importing `sage.all`. This is because of a couple of cyclic imports (e.g. `complex_number` -> `complex_double` -> `complex_number`).
In this ticket, we break these cyclic imports by narrowing down some of the global module imports to local function-wide imports. This was not possible in the case of the `CCtoCDF` class, since there `ComplexDoubleElement` is cimported, which has to be done on the module level. I've hence extracted this class to a new module `complex_conversion.pyx`.
+
+With these changes, the executing the following code
+
+```
+from sage.rings.complex_mpfr import ComplexField
+print(ComplexField())
+```
+using `./venv/bin/python3` works. Previously, it failed with
+
+```
+ File "/home/tobias/sage/src/test.py", line 1, in <module>
+ from sage.rings.complex_mpfr import ComplexField
+ File "sage/rings/complex_mpfr.pyx", line 1, in init sage.rings.complex_mpfr
+ File "sage/rings/real_mpfr.pyx", line 1, in init sage.rings.real_mpfr
+ File "sage/libs/mpmath/utils.pyx", line 17, in init sage.libs.mpmath.utils
+ImportError: cannot import name ComplexField
+```Thanks for the help Matthias!
It still needed a bit further cleanup, but should work now. I've updated the ticket description to discuss what didn't work before but now does.
Thanks, this looks good. I'll test this now also with #32601, but I don't expect problems
Reviewer: Matthias Koeppe
Thanks for the review!
sage -t --long --warn-long 43.1 --random-seed=227602538950590531556014576097185838563 src/sage/geometry/hyperbolic_space/hyperbolic_geodesic.py
**********************************************************************
File "src/sage/geometry/hyperbolic_space/hyperbolic_geodesic.py", line 2038, in sage.geometry.hyperbolic_space.hyperbolic_geodesic.HyperbolicGeodesicPD.plot
Failed example:
H = h.plot(thickness=6, color="orange"); H # optional - sage.plot
Expected:
Graphics object consisting of 2 graphics primitives # optional - sage.plot
Got:
Graphics object consisting of 2 graphics primitives
**********************************************************************
Changed branch from public/refactoring/complexCycle to 401fee5