DOC, API: improve the C-API/Cython documentation and interfaces for random #15007
Conversation
|
The reworked "extending" documentation can be seen here and here is the cython/c-api. The functions in that last link could use some more documentation |
| @@ -112,7 +104,7 @@ The functions are named with the following cconventions: | |||
|
|
|||
| .. c:function:: double random_f(bitgen_t *bitgen_state, double dfnum, double dfden) | |||
|
|
|||
| .. c:function:: double random_standard_cauchy(bitgen_t *bitgen_state) | |||
| .. c:function:: double random_cauchy(bitgen_t *bitgen_state) | |||
There was a problem hiding this comment.
If it already matches the name at the Python level, please leave it as such.
There was a problem hiding this comment.
Reverting the cauchy and random_t changes.
There was a problem hiding this comment.
@mattip I think you forgot to push the revert commit here.
|
I think it would be best, but perhaps not possible, to rename at the python
level. Would also sync with scipy.stats.cauchy
…On Fri, Nov 29, 2019, 18:22 Robert Kern ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In doc/source/reference/random/c-api.rst
<#15007 (comment)>:
> @@ -112,7 +104,7 @@ The functions are named with the following cconventions:
.. c:function:: double random_f(bitgen_t *bitgen_state, double dfnum, double dfden)
-.. c:function:: double random_standard_cauchy(bitgen_t *bitgen_state)
+.. c:function:: double random_cauchy(bitgen_t *bitgen_state)
If it already matches the name at the Python level, please leave it as
such.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#15007?email_source=notifications&email_token=ABKTSROHGKWDEBDZC2SWA4LQWFMVJA5CNFSM4JS44YM2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCNOHKDQ#pullrequestreview-324826382>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKTSRPGNX3CNOE6LKLXHELQWFMVJANCNFSM4JS44YMQ>
.
|
|
Making it harder to upgrade from If anything needs to be done to rationalize this, I'd add a |
I'm not actually arguing these must be changed, although I would argue that it was a mistake to not change then when I personally find standard_t to be a bit confusing since in my area there is a distribution known as the standard ized t that is a Student's t that is normalized to have a unit variance for all choices of the degree of freedom parameter (dof > 2). The t that adds two parameters to set the location and scale is known as a generalized t. |
|
Windows 32bit failures are unrelated. We have I think at least three choices to make with python
I would tend to 2, with no deprecation but with a comment in the documentation. |
| size_t num_colors, int64_t *colors, | ||
| int64_t nsample, | ||
| size_t num_variates, int64_t *variates) | ||
| .. c:function:: int random_mvhg_count(bitgen_t *bitgen_state, npy_int64 total, size_t num_colors, npy_int64 *colors, npy_int64 nsample, size_t num_variates, npy_int64 *variates) |
There was a problem hiding this comment.
I still don't know if mvhg is a sensible name, there's no docs other than the function signature and the name is very non-descriptive. Adding a few lines of docs can be addressed after the 1.18.x split, that would be nice (now it's "read the source code" I think).
For 1.18.x though, can we have a decision on the name? The legacy code has random_hypergeometric, so that's taken. The new code could use hypergeom or multi_hypergeom or something similar perhaps.
There was a problem hiding this comment.
As far as I can see now, random_mvhg_count and random_mvhg_marginals are the only two poor naming choices that are left, everything else looks clear to me.
There was a problem hiding this comment.
@WarrenWeckesser any ideas on nicer names?
There was a problem hiding this comment.
I agree that the names random_mvhg_count and random_mvhg_marginals are not great for a public-facing API. The verbose, completely explicit name for the marginals version would be
random_multivariate_hypergeometric_marginals
but I don't think anyone wants a name that long. Is
random_multivar_hypergeom_marginals
still too long?
There was a problem hiding this comment.
@rgommers wrote:
I still don't know if mvhg is a sensible name, there's no docs other than the function signature
The C functions random_hypergeometric, random_mvhg_count and random_mvhg_marginals have documentation in the comments of the C code. These comments explain the arguments, including assumptions about the input values (e.g. input values are expected to be nonnegative) that are not checked in the C code.
Is the plan to eventually expand the documentation in this file (doc/source/reference/random/c-api.rst) to include such information?
I'm fine with "3. leave it alone". When I brought up the naming inconsistency I missed that those names were already present in the Python API. So I'd say perhaps not worth it; I'd be happy to just have the new additions be named as clearly as possible. |
|
Fixed the niggles and mistakes, tests pass, now waiting for a decision about the names. |
|
I think the first is exceedingly clear. The second is also readable, but
long or long +9 is still a long name.
…On Tue, Dec 3, 2019, 13:37 Warren Weckesser ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In doc/source/reference/random/c-api.rst
<#15007 (comment)>:
>
-.. c:function:: int random_mvhg_count(bitgen_t *bitgen_state,
- int64_t total,
- size_t num_colors, int64_t *colors,
- int64_t nsample,
- size_t num_variates, int64_t *variates)
+.. c:function:: int random_mvhg_count(bitgen_t *bitgen_state, npy_int64 total, size_t num_colors, npy_int64 *colors, npy_int64 nsample, size_t num_variates, npy_int64 *variates)
I agree that the names random_mvhg_count and random_mvhg_marginals are
not great for a public-facing API. The verbose, completely explicit name
for the marginals version would be
random_multivariate_hypergeometric_marginals
but I don't think anyone wants a name that long. Is
random_multivar_hypergeom_marginals
still too long?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#15007?email_source=notifications&email_token=ABKTSROVAPCSFJUPAVB4PWTQWZOHZA5CNFSM4JS44YM2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCNYIQ5A#discussion_r353181076>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKTSRJMJ4Z7TIQJRQ7NESTQWZOHZANCNFSM4JS44YMQ>
.
|
|
Changes look good, so merged. A couple of things left to do or decide in a follow-up PR:
I'd be happy with either, with a light preference for the longer version (so agreeing with @bashtage) |
Clean up the random c documentation
bitgen_tstructRename functions in
distributionsRefactor CFFI test, better document the example files