Skip to content

DOC: fix a failing doctest in DataFrame.to_dict #22827

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

Merged
merged 10 commits into from
Oct 15, 2018

Conversation

Moisan
Copy link
Contributor

@Moisan Moisan commented Sep 25, 2018

  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff

Based on #22459. Fix the docstring for DataFrame.to_dict. I also updated ci/doctests.sh.

@pep8speaks
Copy link

Hello @Moisan! Thanks for submitting the PR.

@codecov
Copy link

codecov bot commented Sep 25, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #22827   +/-   ##
=======================================
  Coverage   92.14%   92.14%           
=======================================
  Files         170      170           
  Lines       51017    51017           
=======================================
  Hits        47011    47011           
  Misses       4006     4006
Flag Coverage Δ
#multiple 90.57% <ø> (ø) ⬆️
#single 42.3% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/frame.py 97.11% <ø> (ø) ⬆️

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 cf11f71...64f6050. Read the comment docs.

Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes, added couple of comments.

@@ -1122,7 +1123,7 @@ def to_dict(self, orient='dict', into=dict):
[{'col1': 1.0, 'col2': 0.5}, {'col1': 2.0, 'col2': 0.75}]

>>> df.to_dict('index')
{'a': {'col1': 1.0, 'col2': 0.5}, 'b': {'col1': 2.0, 'col2': 0.75}}
{'a': {'col1': 1, 'col2': 0.5}, 'b': {'col1': 2, 'col2': 0.75}}
Copy link
Member

Choose a reason for hiding this comment

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

I'm ok merging this and just making the tests pass, but if you want to take care here of making the examples easier to understand, I think it'd be good as in the other strings to use real-world (e.g. animal legs...) examples.

From this, it takes a bit to see what's a, 1... Using something meaningful would be immediate.

Copy link
Member

Choose a reason for hiding this comment

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

@datapythonista I agree with you in general :-), but here I think the 'col1' etc actually makes it easier to see all the different possible output formats (to clearly see where the column names go in the specific output type).
But 'a', 'b' is indeed less clear, but so I would rather call them 'row1', 'row2.

Copy link
Member

Choose a reason for hiding this comment

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

That also makes sense. I'm personally more comfortable with something like {'cat': {'legs': 4, 'tail': 1}} than {'row1': {'col1': 0.75, 'col2': 1}}, but as far as it's not needed to spend a lot of unnecessary time until the example is understood, I'm happy with any option.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry to bother you again with the same @Moisan, but do you mind replacing the labels of the rows by row1 and row2, instead of a and b? I think that will make the example clearer

@jorisvandenbossche
Copy link
Member

Sorry, from looking at the diff now, I don't find this an actual improvement. Eg in the output

{'cat': {'legs': 4, 'wings': 0}, 'falcon': {'legs': 2, 'wings': 2}}

I really have to think hard "are legs/wings the columns or was it cat/falcon ?" to know what the structure is ({index -> {column -> value}} or {column -> {index -> value}})

I find this is easier to see from a glance with something like:

{'row1': {'col1': 4, 'col2': 0}, 'row2': {'col1': 2, 'col2': 2}}

@@ -1085,51 +1085,52 @@ def to_dict(self, orient='dict', into=dict):

Returns
-------
result : collections.Mapping like {column -> {index -> value}}
dict, list or collections.Mapping
like {column -> {index -> value}}
Copy link
Member

Choose a reason for hiding this comment

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

Sorry to ask you one more change. Do you mind changing the description of the Returns to something more accurate? That is only true with the default orient. Also, it'd be nice to start it with a capital letter and end it with a period. Thanks!

Copy link
Contributor Author

@Moisan Moisan Sep 29, 2018

Choose a reason for hiding this comment

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

No problem, better make it right during this pull request than having to come back later!

Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

lgtm. Thanks for another great contribution @Moisan

@datapythonista datapythonista merged commit 6afa51e into pandas-dev:master Oct 15, 2018
@datapythonista
Copy link
Member

thanks @Moisan

@Moisan Moisan deleted the docstring_dataframe_to_dict branch October 15, 2018 17:00
tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request Nov 19, 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.

4 participants