Ticket #3580 (closed defect: fixed)

Opened 2 years ago

Last modified 22 months ago

[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 Author(s):
Report Upstream: Reviewer(s):
Merged in: Work issues:

Description

This is a followup to #3577 that is forced by a merge conflict.

Attachments

trac-3580.patch Download (9.1 KB) - added by craigcitro 22 months ago.

Change History

Changed 22 months ago by mabshoff

  • cc craigcitro, jvoight added

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

Changed 22 months 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")

Changed 22 months 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

Changed 22 months 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.

Changed 22 months 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:

Changed 22 months ago by craigcitro

Changed 22 months ago by craigcitro

Oops. Ticket updated.

Changed 22 months ago by mabshoff

  • status changed from assigned to closed
  • resolution set to fixed

Merged in Sage 3.2.rc0

Note: See TracTickets for help on using tickets.