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

DOC: correct min_count param docstring #19836

Merged
merged 4 commits into from
Feb 23, 2018
Merged

Conversation

ZhuBaohe
Copy link
Contributor

correct min_count param docstring for function ’Series.prod‘, ’Series.sum', 'DataFrame.prod', 'DataFrame.sum'

  • closes #xxxx
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

@jreback jreback added the Docs label Feb 22, 2018
@@ -7867,8 +7867,8 @@ def _doc_parms(cls):

.. versionadded :: 0.22.0

Added with the default being 1. This means the sum or product
of an all-NA or empty series is ``NaN``.
Added with the default being 0. This means the sum or product
Copy link
Contributor

Choose a reason for hiding this comment

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

can you reverse the logic of the final part here. Also can you check the consistency with other text for min_count (in this file)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverse the logic in new commit. If I understand wrong, please give me specific comments. A total of 9 functions share parameter descriptions (Series.sum, Series.prod, Series.product , DataFrame.sum, DataFrame.prod, DataFrame.product , Panel.sum, Panel.prod, Panel.product).

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you find it clearer to phrase the second sentence as

This means the sum of an all-NA or empty Series is 0, and the product of an all-NA or empty Series is 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😄 Already made the change

@codecov
Copy link

codecov bot commented Feb 22, 2018

Codecov Report

Merging #19836 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #19836      +/-   ##
==========================================
+ Coverage   91.57%    91.6%   +0.02%     
==========================================
  Files         150      150              
  Lines       48903    48908       +5     
==========================================
+ Hits        44785    44804      +19     
+ Misses       4118     4104      -14
Flag Coverage Δ
#multiple 89.98% <ø> (+0.02%) ⬆️
#single 41.77% <ø> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/generic.py 95.93% <ø> (ø) ⬆️
pandas/util/testing.py 83.64% <0%> (-0.21%) ⬇️
pandas/core/frame.py 97.23% <0%> (ø) ⬆️
pandas/plotting/_converter.py 66.95% <0%> (+1.73%) ⬆️
pandas/core/reshape/tile.py 92.94% <0%> (+2.68%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b585e3b...22fd6bb. Read the comment docs.

@jorisvandenbossche jorisvandenbossche added this to the 0.23.0 milestone Feb 22, 2018
@pep8speaks
Copy link

pep8speaks commented Feb 22, 2018

Hello @ZhuBaohe! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on February 22, 2018 at 23:16 Hours UTC

@jreback jreback merged commit 7e9ac66 into pandas-dev:master Feb 23, 2018
@jreback
Copy link
Contributor

jreback commented Feb 23, 2018

thanks!

@ZhuBaohe ZhuBaohe deleted the fix branch February 23, 2018 11:44
harisbal pushed a commit to harisbal/pandas that referenced this pull request Feb 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants