Ticket #3580 (closed defect: fixed)
[with patch, with positive review] ensure that totallyreal does not import numpy on startup.
| Reported by: | was | Owned by: | craigcitro |
|---|---|---|---|
| Priority: | blocker | Milestone: | sage-3.2 |
| Component: | misc | Keywords: | |
| Cc: | craigcitro, jvoight | Work issues: | |
| Report Upstream: | Reviewers: | ||
| Authors: | Merged in: | ||
| Dependencies: | Stopgaps: |
Description
This is a followup to #3577 that is forced by a merge conflict.
Attachments
-
trac-3580.patch
(9.1 KB) -
added by craigcitro 5 years ago.
Change History
comment:2 Changed 5 years ago by craigcitro
- Owner changed from cwitty to craigcitro
- Status changed from new to assigned
- Summary changed from ensure that numpy is not imported on startup. to [with patch, needs review] ensure that numpy is not imported on startup.
Moved the import further in, which was fine (didn't slow things down).
Michael, should we add the following as a doctest somewhere?
sage: search_src("^import", "numpy")
sage: search_src("^from", "numpy")
comment:3 Changed 5 years ago by mabshoff
We already have a doctest that is supposed to catch this (via grepping for numpy in "sage -startuptime"), but I think we should add a test in $SAGE_ROOT/devel/sage/tests that looks for the import of numpy in "from sage import all". Right now John's totally real code imports numpy, but the doctest to detect a numpy import at the top level is broken.
What I would like to see is to not import numpy each time, but use something like the mechanism in
http://trac.sagemath.org/sage_trac/attachment/ticket/3498/numpy-3.patch
The mechanism from that patch should be be stuck somewhere in the global namespace and we should enforce its use, i.e. a patch reviewed seeing something like a straight numpy import should complain.
Cheers,
Michael
comment:4 Changed 5 years ago by craigcitro
- Priority changed from major to blocker
- Summary changed from [with patch, needs review] ensure that numpy is not imported on startup. to [with patch, with positive review] ensure that totallyreal does not import numpy on startup.
- Milestone changed from sage-3.2.1 to sage-3.2
This removes the numpy import from the totally real enumeration code, and it fixes a pesky but somewhat serious bug in the code.
John Voight and I team wrote/reviewed this.
I will open a new ticket for the new import of numpy on startup.
comment:5 Changed 5 years ago by mabshoff
There is a slight problem:
sage -t -long devel/sage/sage/rings/number_field/totallyreal_data.pyx
**********************************************************************
File "/scratch/mabshoff/release-cycle/sage-3.2.rc0/devel/sage/sage/rings/number_field/totallyreal_data.pyx", line 200:
sage: [RealField(10)(x) for x in ls]
Expected:
[-1.0000, -1.0000]
Got:
[-1.0, -1.0]
**********************************************************************
1 items had failures:
Currently sage.rings.number_field.totallyreal_data imports numpy at startup. That might be due to the cythonization of totallyreal*, but I am not sure.
Cheers,
Michael