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

ENH: Added submodule numpy.random_intel #8209

Closed

Conversation

oleksandr-pavlyk
Copy link
Contributor

@oleksandr-pavlyk oleksandr-pavlyk commented Oct 24, 2016

numpy.random_intel is MKL-based alternative for numpy.random, first
featured in the Intel Distribution for Python.

It implements all distributions of numpy.random as well as
numpy.random_intel.multinormal_cholesky, which allows for fast
sampling from multinormal distribution when the Cholesky factor
is known ahead of time.

Module numpy.random_intel implements sampling from all distributions
in a vectorized fashion, just like MKL does, which allows MKL to use
vectorized instructions for efficiency.

That means that instead of mtrand's rk_normal which produces a single
variate sampled from normal distribution, mklrand offers vrk_normal_vec,
which produces a vector of variates of requested size.

EDIT: link to mailing list design discussion on this feature: https://mail.scipy.org/pipermail/numpy-discussion/2016-July/075822.html

numpy.random_intel is MKL-based alternative for numpy.random, first
featured in the Intel Distribution for Python.

It implements all distributions of numpy.random as well as
numpy.random_intel.multinormal_cholesky, which allows for fast
sampling from multinormal distribution when the Cholseky factor
is known ahead of time.

Module numpy.random_intel implements sampling from all distributions
in a vectorized fashion, just like MKL does, which allows MKL to use
vectorized instructions for efficiency. That means that instead of
mtrand's rk_normal which produces a single variate sampled from normal
distribution,  mklrand offers vrk_normal_vec, which produces a vector of
variates of requested size.
@oleksandr-pavlyk
Copy link
Contributor Author

@njsmith @rkern

void Py_XINCREF(object obj)

# CObject API
# If this is uncommented it needs to be fixed to use PyCapsule
Copy link
Member

Choose a reason for hiding this comment

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

Take a look at numpy/core/include/numpy/npy_3kcompat.h. Given the current organization, we might want to move that or this work. Not sure what the best organozation will be.

Copy link
Member

@charris charris Oct 26, 2016

Choose a reason for hiding this comment

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

Also note that PyCapsule is available in Python 2.7 and we no longer support earlier versions. In fact, PyCapsule has been used in numpy/random/mtrand/mtrand.pyx since 1.11.x was branched. As long as it is private, it no longer matters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied mtrand/Python.pxi over to `mklrand/Python.pxi'. Thanks!

@charris
Copy link
Member

charris commented Oct 26, 2016

What I notice at first glance are long lines (we like < 80 char/line) and not much documentation. There is doc/C_STYLE_GUIDE.rst.txt for numpy C code, and PEP8 for Python. I'm not sure of the best way to document without too much repetition of current random function documentation. I'm also not sure of the best way will be to handle two distinct modules covering the same material, or whether you should put this work up on PyPI as a separately loadable module -- which was pretty much my first thought.

It would be good to bring this up on the numpy mailing list for discussion: numpy-discussion@scipy.org. @rkern, @njsmith Thoughts?

@rgommers
Copy link
Member

I've edited the PR description to link to the mailing list discussion in July on this.

As far as I can judge from a browse of this PR, this implementation is still standalone and doesn't reflect the design @rkern said he preferred right?

numpy has not been configured with MKL.

Added special case of warning 'ignore' detection to skip
random_intel/__init__.py, parallel to skipping random/__init__.py
@charris
Copy link
Member

charris commented Oct 26, 2016

On Mon, Oct 24, 2016 at 4:03 PM, Oleksandr Pavlyk notifications@github.com
wrote:

@njsmith https://github.com/njsmith @rkern https://github.com/rkern

Hmm, I don't see any replies so far. Other projects that might be
interested are scikit-learn and stats-models.

Chuck

@njsmith
Copy link
Member

njsmith commented Oct 26, 2016

@oleksandr-pavlyk: So you know how this works... usually we use pull request comments to talk about details of code, and for higher-level discussions that affect public API (like: "should numpy have a random_intel module at all?") we switch to the mailing list. That discussion is happening there now, and you probably want to read and participate :-). The mailing list thread starts here: https://mail.scipy.org/pipermail/numpy-discussion/2016-October/076144.html

@rkern
Copy link
Member

rkern commented Oct 27, 2016 via email

@charris
Copy link
Member

charris commented Nov 4, 2016

@oleksandr-pavlyk Sounds like a separate package it is, so closing this. Thanks for your work on this.

@charris charris closed this Nov 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants