-
-
Notifications
You must be signed in to change notification settings - Fork 26k
MNT Use np.asarray to get numpy data type descriptors for C structs #16141
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
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.
Thanks! Related to #15097, LGTM.
Interesting, I wasn't aware of #15097. Since it doesn't look like that pull request is ready to be merged, I think that we should merge this smaller fix first. |
It'd b cleaner to explicitly declare the dtype. That's what we do in sklearn/ensemble/_hist_gradient_boosting/common.pyx for e.g. I'm fine with the proposed way, it's probably not worth nitpicking. Can you please make sure that the resulting dtype is properly aligned to reflect the struct? The struct will be aligned, but numpy dtypes aren't by default. |
I initially sent https://github.com/scikit-learn/scikit-learn/pull/16140/files to clean up the way that field offsets are calculated, but then decided that it would be cleaner (and harder to screw up) to use np.asarray as the comment suggested. |
And yes, the np.asarray method is guaranteed to get the correct alignment. |
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
Thanks @alexhenrie ! |
scikit-learn already requires numpy 1.18 or later.