Opened 5 years ago

Closed 5 years ago

#4571 closed defect (fixed)

[with patch, positive review] merge sage's numpy.pxd with the cython numpy.pxd

Reported by: jason Owned by: mabshoff
Priority: major Milestone: sage-4.1.1
Component: numerical Keywords:
Cc: robertwb, dagss Merged in: Sage 4.1.1.alpha1
Authors: Robert Bradshaw, Dag Sverre Seljebotn Reviewers: Robert Bradshaw, Dag Sverre Seljebotn, Minh Van Nguyen
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

[15:53] <robertwb> yep, we should merge them

Attachments (2)

4571-numpy-pxd.patch (11.5 KB) - added by robertwb 5 years ago.
4571-more-fixes.patch (1.9 KB) - added by dagss 5 years ago.

Download all attachments as: .zip

Change History (19)

comment:2 Changed 5 years ago by robertwb

  • Summary changed from merge sage's numpy.pxd with the cython numpy.pxd to [with patch, needs work] merge sage's numpy.pxd with the cython numpy.pxd

Changed 5 years ago by robertwb

comment:3 Changed 5 years ago by robertwb

Preliminary patch, need a new Cython spkg for it to work all the way.

Changed 5 years ago by dagss

comment:4 follow-up: Changed 5 years ago by dagss

  • Cc dagss added

Here are some more fixes. With this, and the latest state of the

http://hg.cython.org/cython

repo, all the relevant tests seem to pass. I do get two failures (due to Cython upgrade? something else?), see test.log in

/home/dagss/sage-4.0.2-sage.math.washington.edu-x86_64-Linux

Once this works let's tag http://hg.cython.org/cython as 0.11.2.1 for #6438?

comment:5 Changed 5 years ago by dagss

Note (re: discussion on Cython ML) that shape in numpy.pxd is still a field, long story (well, it is performance critical), and I shouldn't change it.

comment:6 Changed 5 years ago by dagss

Another quick comment: With this, full Cython/NumPy? functionality is available from the notebook and works well.

comment:7 Changed 5 years ago by robertwb

Those errors seem completely unrelated, I'll see if I get them too. Other than that, it looks really good.

I've used Cython + NumPy? from the notebook before, as long as you weren't in sage.ext then "cimport numpy" worked as expected (though there was a point where it didn't).

comment:8 Changed 5 years ago by robertwb

Hmm... I also got

The following tests failed:

        sage -t -long devel/sage-main/sage/rings/polynomial/polynomial_template.pxi # 1 doctests failed
        sage -t -long devel/sage-main/sage/interfaces/sage0.py # 1 doctests failed

comment:9 Changed 5 years ago by robertwb

  • Component changed from build to numerical
  • Summary changed from [with patch, needs work] merge sage's numpy.pxd with the cython numpy.pxd to [with patch, positive review] merge sage's numpy.pxd with the cython numpy.pxd

Looks good. The single doctest failure was minor, fixed in #6438.

comment:10 Changed 5 years ago by mvngu

  • Authors set to Robert Bradshaw, Dag Sverre Seljebotn
  • Reviewers set to Robert Bradshaw, Dag Sverre Seljebotn

comment:11 in reply to: ↑ 4 Changed 5 years ago by mvngu

Replying to dagss:

Here are some more fixes. With this, and the latest state of the

<SNIP>
The patch 4571-more-fixes.patch doesn't contain a commit message and your username is not on the patch. Your username should follow the format:

Full Name <email@somewhere.com>

The username makes it easy to identify the patch, when it is committed, as your contribution.

comment:12 Changed 5 years ago by mvngu

  • Summary changed from [with patch, positive review] merge sage's numpy.pxd with the cython numpy.pxd to [with patch, needs work] merge sage's numpy.pxd with the cython numpy.pxd

With both patches applied in the following order:

  1. 4571-numpy-pxd.patch
  2. 4571-more-fixes.patch

I see the following build failure:

[mvngu@sage sage-4.1.1.alpha0]$ ./sage -br main

----------------------------------------------------------
sage: Building and installing modified Sage library files.


Installing c_lib
scons: `install' is up to date.
Updating Cython code....
Building modified file sage/finance/time_series.pyx.
Building modified file sage/matrix/change_ring.pyx.
Building modified file sage/matrix/matrix_complex_double_dense.pyx.
Building modified file sage/matrix/matrix_double_dense.pyx.
Building modified file sage/matrix/matrix_real_double_dense.pyx.
Building modified file sage/modules/vector_complex_double_dense.pyx.
Building modified file sage/modules/vector_double_dense.pyx.
Building modified file sage/modules/vector_real_double_dense.pyx.
Building sage/rings/polynomial/real_roots.pyx because it depends on sage/modules/vector_double_dense.pxd.
Building sage/stats/hmm/chmm.pyx because it depends on sage/matrix/matrix_double_dense.pxd.
Building sage/stats/hmm/hmm.pyx because it depends on sage/matrix/matrix_double_dense.pxd.
python `which cython` --embed-positions --incref-local-binop -I/scratch/mvngu/release/sage-4.1.1.alpha0/devel/sage-main -o sage/finance/time_series.c sage/finance/time_series.pyx
warning: /scratch/mvngu/release/sage-4.1.1.alpha0/devel/sage-main/sage/finance/time_series.pyx:1722:24: cdef variable 'j' declared after it is used

Error converting Pyrex file to C:
------------------------------------------------------------
...
            [20.0000, -3.0000, 4.5000, -2.0000]
        """
        cnumpy.import_array() #This must be called before using the numpy C/api or you will get segfault
        cdef cnumpy.npy_intp dims[1]
        dims[0] = self._length
        cdef cnumpy.ndarray n = cnumpy.PyArray_SimpleNewFromData(1, dims, cnumpy.NPY_DOUBLE, self._values)
                                                                       ^
------------------------------------------------------------

/scratch/mvngu/release/sage-4.1.1.alpha0/devel/sage-main/sage/finance/time_series.pyx:1862:72: Cannot convert 'numpy.npy_intp [1]' to Python object

Error converting Pyrex file to C:
------------------------------------------------------------
...
            [20.0000, -3.0000, 4.5000, -2.0000]
        """
        cnumpy.import_array() #This must be called before using the numpy C/api or you will get segfault
        cdef cnumpy.npy_intp dims[1]
        dims[0] = self._length
        cdef cnumpy.ndarray n = cnumpy.PyArray_SimpleNewFromData(1, dims, cnumpy.NPY_DOUBLE, self._values)
                                                                                                ^
------------------------------------------------------------

/scratch/mvngu/release/sage-4.1.1.alpha0/devel/sage-main/sage/finance/time_series.pyx:1862:97: Cannot convert 'double *' to Python object
Error running command, failed with status 256.
sage: There was an error installing modified sage library code.

comment:13 follow-up: Changed 5 years ago by robertwb

Looks like it'll have to be rebased, worked fine on vanilla 4.1. Where's the .tar on sage.math?

comment:14 in reply to: ↑ 13 Changed 5 years ago by mvngu

Replying to robertwb:

Looks like it'll have to be rebased, worked fine on vanilla 4.1. Where's the .tar on sage.math?

Source and binary versions are up at

http://sage.math.washington.edu/home/mvngu/release/sage-4.1.1.alpha0.tar
http://sage.math.washington.edu/home/mvngu/release/sage-4.1.1.alpha0-sage.math.washignton.edu-x86_64-Linux.tar.gz

comment:15 Changed 5 years ago by robertwb

Works fine form me. sage-4.1.1.alpha0-sage.math.washignton.edu-x86_64-Linux.tar.gz + #6438 + #4571. Are you sure applied the latest Cython spkg first? 4.1.1.alpha0 doesn't seem to have it.

comment:16 Changed 5 years ago by robertwb

  • Summary changed from [with patch, needs work] merge sage's numpy.pxd with the cython numpy.pxd to [with patch, needs review] merge sage's numpy.pxd with the cython numpy.pxd

comment:17 Changed 5 years ago by mvngu

  • Merged in set to Sage 4.1.1.alpha1
  • Resolution set to fixed
  • Reviewers changed from Robert Bradshaw, Dag Sverre Seljebotn to Robert Bradshaw, Dag Sverre Seljebotn, Minh Van Nguyen
  • Status changed from new to closed
  • Summary changed from [with patch, needs review] merge sage's numpy.pxd with the cython numpy.pxd to [with patch, positive review] merge sage's numpy.pxd with the cython numpy.pxd

dagss: The patch 4571-more-fixes.patch doesn't contain your username. I'm committing it in your name. Merged in this order

  1. the SPKG at #6438
  2. 4571-numpy-pxd.patch
  3. 4571-more-fixes.patch
Note: See TracTickets for help on using tickets.