Repository navigation
Document MKLRandomState in the API reference and align docstrings with NumPy - #165
vlad-perevezentsev wants to merge 12 commits into
Conversation
|
View rendered docs @ https://intelpython.github.io/mkl_random/pull/165/index.html |
|
mkl_random/mkl_random/mklrand.pyx Line 6726 in 52ea3c9 Should we also change here to |
jharlow-intel
left a comment
jharlow-intel
left a comment
There was a problem hiding this comment.
Thanks, this is a big step up for the API docs! Grouping the methods into autosummary tables and switching to numpydoc makes the reference much easier to navigate, and the docstrings now say clearly which arguments broadcast. A couple of things to sort out before merge; details inline.
- Heads-up that this now conflicts with master in
mklrand.pyx(from #189'smultinormal_choleskyvalidation), and #192, which is approved, also touches thebytesandtomaxintdocstrings. A rebase after #192 lands should sort both out. #107 overlapsapi.rstandconf.pytoo, so it's probably easiest if this goes in before it.
| with: | ||
| name: ${{ env.PACKAGE_NAME }} rendered documentation | ||
| path: ~/rendered_docs | ||
| # TEMPORARY: publish this PR's docs to gh-pages and comment the link. |
There was a problem hiding this comment.
I think the temporary preview steps need to come out before this merges, as the comment here says. Besides pushing each PR's docs to the public gh-pages with a write token, the mshick/add-pr-comment pin is what fails the zizmor check: its # v2 comment doesn't match the pinned SHA (v2 currently points at b8f338c).
| scale : float | ||
| scale : float or array_like of floats | ||
| Standard deviation (spread or "width") of the distribution. | ||
| Must be non-negative. |
There was a problem hiding this comment.
Small accuracy thing from aligning with NumPy: mkl_random rejects zero here (raise ValueError("scale <= 0")), while NumPy allows it, so "Must be non-negative" isn't quite right. The same goes for exponential, gamma, laplace, gumbel, logistic, and lognormal's sigma. Could we say "Must be positive" in those?
| method : {'ICDF', 'BoxMuller', 'BoxMuller2'}, optional | ||
| Sampling method used by Intel MKL. Can also be specified using | ||
| tokens mkl_random.ICDF, mkl_random.BOXMULLER, mkl_random.BOXMULLER2 | ||
| the tokens ``mkl_random.mklrand.ICDF``, |
There was a problem hiding this comment.
Nit: the public names are mkl_random.ICDF, mkl_random.BOXMULLER, and mkl_random.BOXMULLER2 (they're exported at the top level), so it might be friendlier to point at those than at mkl_random.mklrand.
Per review, older than this PR but in the lines being touched: the Parameters block described loc and scale, which the function does not take, and never mentioned p, while Returns claimed the values lie in [0, n]. The support is the positive integers. gh-165 rewrites this block as well, so that PR keeps the final wording; the range there needs to read [0, 1) after this one.
This branch has not been deployed
3 participants
This PR documents
MKLRandomStatetogether with its methods, and renders it withnumpydocfor consistency with NumPy's docs.Previously the API reference only showed the deprecated
RandomStatealias, without its methods.MKLRandomStateand its methods via per-methodautosummarypages.array_likeparameters, fix wrong parameter types/names, add missingReturnssections and constraint/default notes.