-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
[MRG] Fix #12363 Included criterion.pxd #12381
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
Conversation
sklearn/tree/setup.py
Outdated
@@ -31,7 +31,7 @@ def configuration(parent_package="", top_path=None): | |||
extra_compile_args=["-O3"]) | |||
|
|||
config.add_subpackage("tests") | |||
|
|||
config.add_data_files('_criterion.pxd') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add the other pxd files here
sklearn/tree/setup.py
Outdated
@@ -31,7 +31,11 @@ def configuration(parent_package="", top_path=None): | |||
extra_compile_args=["-O3"]) | |||
|
|||
config.add_subpackage("tests") | |||
config.add_data_files('_criterion.pxd') | |||
config.add_data_files("_tree.pxd") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alphabetical order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mean I should add pxd files in alphabetical order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks. From a code perspective this LGTM, but I should check the wheel when I'm not mobile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, scipy does this for cython_blas.pxd
so it should be fine, I think.
Checked that these are included in the generated wheel. Merging. Thanks! |
@RoopamSharma we're thinking of using memory views in the tree code, which implies some changes to the criterion and splitter's interface. What do you think about the changes proposed in #12886? |
Hi,
I'm not aware with the code. I just added the .pxd files as my first GitHub
commit.
Thanks & Regards
Roopam Sharma
…On Tue, Jan 15, 2019, 7:05 PM Adrin Jalali ***@***.*** wrote:
@RoopamSharma <https://github.com/RoopamSharma> we're thinking of using
memory views in the tree code, which implies some changes to the criterion
and splitter's interface. What do you think about the changes proposed in
#12886 <#12886>?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#12381 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHE5NL-rFXg1MtiOGm0zlySC7nyUgxFBks5vDdksgaJpZM4XbPZ0>
.
|
…it-learn#12381)" This reverts commit 0736a17.
…it-learn#12381)" This reverts commit 0736a17.
Fixes #12363
Included _criterion.pxd in the sklearn/tree/setup.py wheel to derive new criterion