Skip to content

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

Merged
merged 1 commit into from
Jan 27, 2020
Merged

Conversation

alexhenrie
Copy link
Contributor

scikit-learn already requires numpy 1.18 or later.

Copy link
Member

@rth rth left a 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.

@alexhenrie
Copy link
Contributor Author

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.

@NicolasHug
Copy link
Member

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. HISTOGRAM_DTYPE and hist_struct.

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.

@alexhenrie
Copy link
Contributor Author

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.

@alexhenrie
Copy link
Contributor Author

And yes, the np.asarray method is guaranteed to get the correct alignment.

@rth rth requested a review from jeremiedbb January 27, 2020 08:38
Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

lgtm

@jeremiedbb jeremiedbb merged commit b7c4f4f into scikit-learn:master Jan 27, 2020
@jeremiedbb
Copy link
Member

Thanks @alexhenrie !

thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Feb 22, 2020
panpiort8 pushed a commit to panpiort8/scikit-learn that referenced this pull request Mar 3, 2020
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