8000 Split binary_tree.pxi into binary_tree.pyx + binary_tree.pxd and import rather than include · Issue #6409 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

Split binary_tree.pxi into binary_tree.pyx + binary_tree.pxd and import rather than include #6409

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.

Sign up for GitHub

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

Jump to bottom
Open
raghavrv opened this issue Feb 19, 2016 · 1 comment
Open

Split binary_tree.pxi into binary_tree.pyx + binary_tree.pxd and import rather than include #6409

raghavrv opened this issue Feb 19, 2016 · 1 comment
Labels
cython module:neighbors Needs Decision Requires decision

Comments

@raghavrv
Copy link
Member

There is a pxi file at neighbors/binary_tree.pxi

I may be wrong here, but isn't importing a pyx rather than including the pxi a better/cleaner way? Or is it just the same? (If so is there a specific reason why that won't work here?)

Also the way the current cythonize.py is setup, it will track for changes in pyx/pxd files alone and recythonize them if changed.

If the binary_tree.pxi is to be untouched, we need a minor hack to make it compile the kd_tree.pyx and ball_tree.pyx whenever the binary_tree.pxi is modified. (Also see #6254)

If the binary_tree.pxi can be split into binary_tree.pyx + binary_tree.pxd, I'd like to take it up as I am trying to learn cython. (For fiddling with the sklearn/tree module)

cc: @jakevdp @amueller @ogrisel @MechCoder
@jakevdp
Copy link
Member
jakevdp commented Feb 20, 2016

When I wrote this code, I initially started with a pyx/pxd solution, but I found that the standard inheritence scheme for some reason led to massive slowdowns in queries, because of the details of cython's vtable lookups IIRC. That was several years ago, so Cython may have improved things since then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
cython module:neighbors Needs Decision Requires decision
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jakevdp @cmarmo @raghavrv
0