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: Improve the docsting of Series.iteritems #24879

Merged
merged 8 commits into from Mar 19, 2019

Conversation

HubertKl
Copy link
Contributor

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

@codecov
Copy link

codecov bot commented Jan 22, 2019

Codecov Report

Merging #24879 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #24879   +/-   ##
=======================================
  Coverage   92.39%   92.39%           
=======================================
  Files         166      166           
  Lines       52410    52410           
=======================================
  Hits        48422    48422           
  Misses       3988     3988
Flag Coverage Δ
#multiple 90.81% <ø> (ø) ⬆️
#single 42.88% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/series.py 93.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 853cd70...92ab782. Read the comment docs.

@codecov
Copy link

codecov bot commented Jan 22, 2019

Codecov Report

Merging #24879 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24879      +/-   ##
==========================================
- Coverage   91.25%   91.25%   -0.01%     
==========================================
  Files         172      172              
  Lines       52977    52977              
==========================================
- Hits        48343    48342       -1     
- Misses       4634     4635       +1
Flag Coverage Δ
#multiple 89.82% <ø> (ø) ⬆️
#single 41.74% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/series.py 93.68% <ø> (ø) ⬆️
pandas/util/testing.py 88.98% <0%> (-0.1%) ⬇️

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 65c0441...15735ef. Read the comment docs.

Copy link
Contributor

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

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

Looks nice overall.

I'm not entirely sure about the focus on "creating a new series". I think the thing to focus on is other functions that can only take scalar inputs (are not vectorized). You can do many things with the output of the non-vectorized method (including creating a new series).

@@ -1446,6 +1446,50 @@ def to_string(self, buf=None, na_rep='NaN', float_format=None, header=True,
def iteritems(self):
"""
Lazily iterate over (index, value) tuples.

This method returns a zip of tuples (index, value). This is useful When
Copy link
Contributor

Choose a reason for hiding this comment

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

How about "iterable" instead of "zip"?

@@ -1446,6 +1446,50 @@ def to_string(self, buf=None, na_rep='NaN', float_format=None, header=True,
def iteritems(self):
"""
Lazily iterate over (index, value) tuples.

This method returns a zip of tuples (index, value). This is useful When
one want to create new series from the values of an old one. Be aware
Copy link
Contributor

Choose a reason for hiding this comment

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

"one wants" (but "you want" probably sounds better)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about creating a new series using itertuples. I'd rather say something about lazily iterating over the index, value pairs.

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 am not sure to perfectly get what you mean. I tried to add an example where vectorization is not possible (as far as I know maybe I am wrong)


Returns
-------
zip
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if people have thoughts here. I guess zip is technically a type... But I think Iterable is probably better-known.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. I think iterable would be more user-friendly.

Returns
-------
zip
Iterable tuples (index, value) of the Series.
Copy link
Contributor

Choose a reason for hiding this comment

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

Iterable of tuples containing the (index, value) pairs from a Series.

@gfyoung gfyoung added the Docs label Jan 23, 2019
@HubertKl
Copy link
Contributor Author

Thanks for those comments, I tried to improve the docstring accordingly to your remarks.

@@ -1446,6 +1446,30 @@ def to_string(self, buf=None, na_rep='NaN', float_format=None, header=True,
def iteritems(self):
"""
Lazily iterate over (index, value) tuples.

This method returns an iterable tuple (index, value). This is
convienient if you want to create a a Lazy iterator.
Copy link
Member

Choose a reason for hiding this comment

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

There are 2 as on this line. Lowercase l on Lazy


See Also
--------
Series.apply : Invoke function on values of Series.
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if Series.apply and Series.map are too related to this method. I would instead reference Series.items

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless I am mistaken, it seems to me that Series.iteritems and Series.items are the same method. The docstring of items and iteritems will be the same I don't think that this is a good idea.

I will mention it in the description instead.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that apply and map aren't closely enough related to include here - can you remove?

@pep8speaks
Copy link

pep8speaks commented Jan 31, 2019

Hello @HubertKl! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-03-16 10:50:43 UTC

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

Minor comment otherwise lgtm


See Also
--------
Series.apply : Invoke function on values of Series.
Copy link
Member

Choose a reason for hiding this comment

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

I agree that apply and map aren't closely enough related to include here - can you remove?

@TomAugspurger
Copy link
Contributor

@HubertKl can you update, removing Series.apply and Series.map in the See Also?

@jreback
Copy link
Contributor

jreback commented Mar 10, 2019

@HubertKl can you update to comments

@HubertKl
Copy link
Contributor Author

Sorry for the delay. I modified the PR accordingly to the requests.

@WillAyd WillAyd added this to the 0.25.0 milestone Mar 19, 2019
@WillAyd WillAyd merged commit 406e7ea into pandas-dev:master Mar 19, 2019
@WillAyd
Copy link
Member

WillAyd commented Mar 19, 2019

Thanks @HubertKl !

sighingnow added a commit to sighingnow/pandas that referenced this pull request Mar 20, 2019
* origin/master:
  DOC: clean bug fix section in whatsnew (pandas-dev#25792)
  DOC: Fixed PeriodArray api ref (pandas-dev#25526)
  Move locale code out of tm, into _config (pandas-dev#25757)
  Unpin pycodestyle (pandas-dev#25789)
  Add test for rdivmod on EA array (GH23287) (pandas-dev#24047)
  ENH: Support datetime.timezone objects (pandas-dev#25065)
  Cython language level 3 (pandas-dev#24538)
  API: concat on sparse values (pandas-dev#25719)
  TST: assert_produces_warning works with filterwarnings (pandas-dev#25721)
  make core.config self-contained (pandas-dev#25613)
  CLN: replace %s syntax with .format in pandas.io.parsers (pandas-dev#24721)
  TST: Check pytables<3.5.1 when skipping (pandas-dev#25773)
  DOC: Fix typo in docstring of DataFrame.memory_usage  (pandas-dev#25770)
  Replace dicts with OrderedDicts in groupby aggregation functions (pandas-dev#25693)
  TST: Fixturize tests/frame/test_missing.py (pandas-dev#25640)
  DOC: Improve the docsting of Series.iteritems (pandas-dev#24879)
  DOC: Fix function name. (pandas-dev#25751)
  Implementing iso_week_year support for to_datetime (pandas-dev#25541)
  DOC: clarify corr behaviour when using a callable (pandas-dev#25732)
  remove unnecessary check_output (pandas-dev#25755)

# Conflicts:
#	doc/source/whatsnew/v0.25.0.rst
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.

None yet

7 participants