-
Notifications
You must be signed in to change notification settings - Fork 282
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
Appended to docu-string for function make_array and updated function … #588
Conversation
…sample_proportions Added args and return type to docu-string for function make_array. Also updated function sample_proportions to use multinomial method of Generator class per Numpy recommendations: https://numpy.org/doc/stable/reference/random/generated/numpy.random.multinomial.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, looks good - thanks for these changes! Please revise as per the comments and then it should be good to merge. Thanks!
@@ -16,14 +16,19 @@ | |||
import collections | |||
|
|||
# Change matplotlib formatting. TODO incorporate into a style? | |||
plt.rcParams["patch.force_edgecolor"] = True | |||
plt.rcParams['patch.force_edgecolor'] = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure why this change is required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of single quotes maintains consistency with the rest of the codebase where single quotes are used. Honestly, maybe not necessary but might as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. Let's leave your changes then :)
datascience/util.py
Outdated
Args: | ||
``elements`` (variadic): elements | ||
Returns: | ||
An array of same length as the provided varadic argument ``elements`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe clarify that this is a NumPy array since Python does not have a native array representation type
datascience/util.py
Outdated
@@ -84,7 +89,7 @@ def plot_normal_cdf(rbound=None, lbound=None, mean=0, sd=1): | |||
|
|||
``mean`` (numeric): mean/expectation of normal distribution | |||
|
|||
``sd`` (numeric): standard deviation of normal distribution | |||
``sd`` (numeric): standard deviation of normal distribution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure why this is required
@@ -254,4 +260,4 @@ def is_non_string_iterable(value): | |||
return False | |||
if hasattr(value, '__iter__'): | |||
return True | |||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please maintain the extra line at the end of the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this change :)
Clarified that function make_array returns NumPy array. Added empty line to end of file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the requested changes - a quality PR! :D
@@ -16,14 +16,19 @@ | |||
import collections | |||
|
|||
# Change matplotlib formatting. TODO incorporate into a style? | |||
plt.rcParams["patch.force_edgecolor"] = True | |||
plt.rcParams['patch.force_edgecolor'] = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. Let's leave your changes then :)
@@ -254,4 +260,4 @@ def is_non_string_iterable(value): | |||
return False | |||
if hasattr(value, '__iter__'): | |||
return True | |||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this change :)
[ ] Wrote test for feature
[ ] Added changes to CHANGELOG.md
[ ] Bumped version number (delete if unneeded)
Changes proposed:
Added args and return type to docu-string for function make_array. Also updated function sample_proportions to use multinomial method of Generator class per Numpy recommendations: https://numpy.org/doc/stable/reference/random/generated/numpy.random.multinomial.html