TST, DOC: Added Broadcasting Tests in test_random.py #7082
Conversation
| @@ -3426,7 +3426,8 @@ cdef class RandomState: | |||
| Returns | |||
| ------- | |||
| samples : ndarray or scalar | |||
| where the values are all integers in [0, n]. | |||
| Samples from logistic distribution, shaped according to | |||
| `size`. Otherwise, a single value is returned. | |||
There was a problem hiding this comment.
is the "Otherwise..." really needed? I feel that given the explanation on the size parameter, this is more confusing than helpful. Also, size=None does not necessarily mean a single return if the other parameters are arrays, right? Again, I'd make that change in size, not here.
There was a problem hiding this comment.
Indeed, that is true. I think the documentation needs to be more standardized IMO, so I'll go through the doc strings for these broadcasting functions to make sure that that they properly explain how the size of the output varies depending on size and the size of the inputs.
There was a problem hiding this comment.
@jaimefrio : Docs all updated.
|
Travis + Appveyor are happy, and the broadcasting + documentation (I think) have been hammered out pretty thoroughly now. Ready to merge. |
Added a whole new suite of tests to ensure that functions in mtrand.pyx which are broadcastable actually broadcast their arguments properly.
| single value is returned. | ||
| ``m * n * k`` samples are drawn. If size is ``None`` (default), | ||
| a single value is returned if ``scale`` is a scalar. Otherwise, | ||
| ``len(scale)`` samples are drawn. |
There was a problem hiding this comment.
This is not exactly correct: if scale is a 2D array, len(scale) is its number of rows, and this function returns scale.size items. If we wanted to write it in code, np.array(scale).size is probably the most accurate representation, but it may be better to come up with a description in prose, not sure...
There was a problem hiding this comment.
Done. I prefer np.array(scale).size because it's more concise and should get the point across clear enough given the intended audience. I had tried to describe these size-related values in more prose when working on this PR, but I couldn't come up with anything clear enough.
Clarified the output size depending on whether scalar or non-scalar inputs are passed to functions in mtrand.pyx that can broadcast their arguments.
TST, DOC: Added Broadcasting Tests in test_random.py
|
Thanks for your patience with my finding incremental nitpicks, in it goes! |
|
No problem. It's easier to fix a PR before it's merged than after. 😄 |
Follow-up from #6997 in which I found that there was almost no testing for the broadcasting behavior of methods defined to broadcast in
mtrand.pyx. Clarified documentation for these broadcastable functions with regards to output size depending on whethersizeisNoneand whether the inputs are all scalars.