Skip to content

MAINT: Use uint8_t from numpy instead of stdint.h#295

Merged
jvkersch merged 3 commits into
masterfrom
maint/avoid-stdint-h
Oct 30, 2017
Merged

MAINT: Use uint8_t from numpy instead of stdint.h#295
jvkersch merged 3 commits into
masterfrom
maint/avoid-stdint-h

Conversation

@jvkersch

Copy link
Copy Markdown
Contributor

The Cython module kiva._cython_speedups was using the stdint.h header, which is not shipped with VS 2008 (needed for Python 2.7). This replaces it by the equivalent declaration from Numpy.

Comment thread kiva/_cython_speedups.pyx Outdated
cdef double[:, ::1] pts_view = pts
cdef double[:, ::1] poly_pts_view = poly_pts
cdef uint8_t[::1] results = np.zeros(len(pts), dtype=np.uint8)
cdef np.uint8_t[::1] results = np.zeros(len(pts), dtype=np.uint8)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that shouldn't be np.npy_uint8?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that magically transmogrifies to npy_uint8 somewhere deep under the Cython hood...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Thanks for clearing that up

Comment thread kiva/_cython_speedups.pyx Outdated
@@ -1,5 +1,5 @@
from libc.stdint cimport uint8_t
import numpy as np
cimport numpy as np

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit: I may be alone in this (certainly the Cython numpy tutorials do it like you do), but I don't like import numpy as np and cimport numpy as np. I usually cimport numpy as cnp to distinguish the later references.

Or in this case, since you are importing only one thing: from numpy cimport uint8_t

Either way, LGTM.

@jwiggins

Copy link
Copy Markdown
Member

We should probably get AppVeyor hooked up for this project...

@jvkersch

Copy link
Copy Markdown
Contributor Author

We should probably get AppVeyor hooked up for this project...

Agreed, and same thing for Chaco. I ran into this issue (and a similar issue for Chaco) while trying to build Python 2.7 eggs on Windows...

@codecov-io

codecov-io commented Oct 30, 2017

Copy link
Copy Markdown

Codecov Report

Merging #295 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #295   +/-   ##
======================================
  Coverage    33.6%   33.6%           
======================================
  Files         206     206           
  Lines       18278   18278           
  Branches     2407    2407           
======================================
  Hits         6142    6142           
  Misses      11744   11744           
  Partials      392     392

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e19330...ae34ae3. Read the comment docs.

@jvkersch

Copy link
Copy Markdown
Contributor Author

Thanks guys, merging.

@jvkersch jvkersch merged commit a325b5b into master Oct 30, 2017
@jvkersch jvkersch deleted the maint/avoid-stdint-h branch October 30, 2017 21:54
jvkersch added a commit that referenced this pull request Nov 14, 2017
MAINT: Use uint8_t from numpy instead of stdint.h
jvkersch added a commit that referenced this pull request Nov 15, 2017
Merge pull request #295 from enthought/maint/avoid-stdint-h
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants