Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CLN] More Misc Cleanups in _libs #22287

Merged
merged 9 commits into from
Aug 20, 2018
Merged

Conversation

jbrockmendel
Copy link
Member

cpdef --> cdef in a few cases
modernize string formatting
modernize for loop syntax

Should be orthogonal to other outstanding PR(s)

@@ -647,7 +646,7 @@ def row_bool_subset(ndarray[float64_t, ndim=2] values,
out[pos, j] = values[i, j]
pos += 1

return out
return out.base # `.base` to access underlying np.ndarray
Copy link
Contributor

Choose a reason for hiding this comment

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

I know elsewhere we do np.asarray(<memoryview>) - are those equivalent?

Copy link
Member Author

Choose a reason for hiding this comment

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

Very close to equivalent. It's a bit roundabout.

Above out is typed as a float64_t[:, :] memoryview, but then instantiated with

out = np.empty((mask.sum(), k), dtype=np.float64)

In executing that line, cython creates the numpy array, then casts it as a memoryview out, and attaches the original ndarray as out.base. So out.base is an attribute lookup to return an existing np.ndarray.

The performance penalty to np.asarray must be tiny; it might be worth it to do it that way to avoid confusion.

Copy link
Contributor

@jreback jreback Aug 13, 2018

Choose a reason for hiding this comment

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

ahh yes, can we change these

@codecov
Copy link

codecov bot commented Aug 12, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #22287   +/-   ##
=======================================
  Coverage   92.05%   92.05%           
=======================================
  Files         169      169           
  Lines       50709    50709           
=======================================
  Hits        46679    46679           
  Misses       4030     4030
Flag Coverage Δ
#multiple 90.46% <ø> (ø) ⬆️
#single 42.25% <ø> (ø) ⬆️

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 70e6f7c...bcff47a. Read the comment docs.

@gfyoung gfyoung added Internals Related to non-user accessible pandas implementation Clean labels Aug 13, 2018
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

looks fine. are you linting these? (obviously how you made some of the changes), is this thru an IDE? I know I have asked this a bunch of times.

@jbrockmendel
Copy link
Member Author

are you linting these? (obviously how you made some of the changes)

No, just periodically reading through them. My vague plan for achieving linting is to get the code (at least some pyx modules) "close enough" to valid python to trick the linter.

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

if you'd fix the .base things, otherwise lgtm.

@@ -329,10 +329,10 @@ def fast_zip(list ndarrays):
Py_INCREF(val)
PyArray_ITER_NEXT(it)

return result
return result.base # `.base` to access underlying np.ndarray
Copy link
Contributor

Choose a reason for hiding this comment

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

can you change these in this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

@jbrockmendel
Copy link
Member Author

Had to revert some edits to fix persistent ResourceWarning in one Travis environment. Will try to narrow down the culprit in the next pass.

@jbrockmendel
Copy link
Member Author

Ping (got “LGTM” a few days ago)

@jreback jreback added this to the 0.24.0 milestone Aug 20, 2018
@jreback jreback merged commit b6e35ff into pandas-dev:master Aug 20, 2018
@jreback
Copy link
Contributor

jreback commented Aug 20, 2018

thanks

@jbrockmendel jbrockmendel deleted the cln_more branch August 24, 2018 04:07
Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Internals Related to non-user accessible pandas implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants