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: add dtype option to numpy.lib.function_base.cov and corrcoef #17456

Merged
merged 15 commits into from
Oct 9, 2020

Conversation

lschwetlick
Copy link
Contributor

@lschwetlick lschwetlick commented Oct 5, 2020

I tried to implement the changes suggested in Issue #17395, by adding a dtype keyword to the cov and corrcoef functions in numpy/lib/function_base.py. I also wrote tests for the changes.

Closes #17395

Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

This generally looks good, thanks for the contribution. Two more things:

  • The dtype argument needs documenting, and should include .. versionadded:: 1.20.0
  • We should maybe add a release note in the upcoming_changes folder.

@lschwetlick
Copy link
Contributor Author

Sure, I can certainly do that. I left it out initially to see whether there was interest in actually merging the PR. Thank you for reviewing so quickly @eric-wieser !

Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

This probably doesn't need to hit the mailing list, but I'll leave it for another maintainer to give a second opinion on. Thanks for the rapid iteration.

numpy/lib/function_base.py Outdated Show resolved Hide resolved
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
@lschwetlick
Copy link
Contributor Author

Happy to help!

Its a bit silly maybe but if its no trouble, could you add the label "hacktoberfest-accepted" to the PR? They changed the rules to prevent spam.

@eric-wieser
Copy link
Member

eric-wieser commented Oct 5, 2020

@lschwetlick, there's no need for me to do that according to your link, my approval above is enough.

What does matter is if the repo has a hacktoberfest topic, which it currently does not. I'd be happy to add it to make this PR count, but I worry that we might draw spam attention as well.

@lschwetlick
Copy link
Contributor Author

Yup, I get it. I mainly liked the gamification aspect as a motivator. Good use of my time anyway :)

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

LGTM as well, seems like a nice improvement. Thanks @lschwetlick !

numpy/lib/function_base.py Outdated Show resolved Hide resolved
@rossbar
Copy link
Contributor

rossbar commented Oct 9, 2020

I've updated the docstring wording based on @seberg 's comment (and undid my test changes since I was in there). I'll merge once the CI's done - I don't want to let wording block this nice new feature (thanks @lschwetlick !) We can always revisit the docs later to improve the wording or add more explanation if needed.

@rossbar rossbar merged commit 156cd05 into numpy:master Oct 9, 2020
@charris charris mentioned this pull request Oct 10, 2020
20 tasks
@eric-wieser
Copy link
Member

eric-wieser commented Oct 23, 2020

Added the hacktoberfest-accepted label, since I think @mattip said that was preferable to the repo topic. Apologies if it's too late to make a difference!

@lschwetlick
Copy link
Contributor Author

Nice, thank you @eric-wieser !

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.

dtype control for corrcoef
4 participants