DOC: Improving docstring of take method by matagus · Pull Request #16948 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation32 Commits2 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
- closes #xxxx
- tests added / passed
- passes
git diff upstream/master -u -- "*.py" | flake8 --diff - whatsnew entry
Hello @matagus! Thanks for updating the PR.
Cheers ! There are no PEP8 issues in this Pull Request. 🍻
Comment last updated on August 21, 2017 at 08:25 Hours UTC
| @@ -1939,7 +1939,8 @@ def __delitem__(self, key): |
|---|
| def take(self, indices, axis=0, convert=True, is_copy=True, **kwargs): |
| """ |
| Analogous to ndarray.take |
| Return an object formed from the elements in the given indices along an |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally try to fit this description on one line.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| @@ -1948,6 +1949,44 @@ def take(self, indices, axis=0, convert=True, is_copy=True, **kwargs): |
|---|
| convert : translate neg to pos indices (default) |
| is_copy : mark the returned frame as a copy |
| Examples |
| -------- |
| >>> import numpy as np |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need the imports here
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| @@ -1939,7 +1939,8 @@ def __delitem__(self, key): |
|---|
| def take(self, indices, axis=0, convert=True, is_copy=True, **kwargs): |
| """ |
| Analogous to ndarray.take |
| Return an object formed from the elements in the given indices along an |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add that this is a positional selection.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| name class max_speed |
|---|
| 3 monkey mammal NaN |
| 2 lion mammal 80.5 |
| Returns |
| ------- |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a See Also, showing ndarray.take
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| columns=('name', 'class', 'max_speed')) |
|---|
| >>> df |
| name class max_speed |
| 0 falcon bird 389.0 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a comment that these are positional selections (where you think it is needed)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| >>> import numpy as np |
|---|
| >>> import pandas as pd |
| >>> df = pd.DataFrame([('falcon', 'bird', 389.0), |
| ('parrot', 'bird', 24.0), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
give this an index (like [0, 3, 2, 1]) or something to emphasize that these are positional operations
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@matagus Do you have time to update this according to the feedback?
(if not, no problem, but please indicate this so somebody else can take over)
@gfyoung if you have a chance can you fix up.
@jreback : Changes requested have been addressed. PTAL.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Some nitpicks
| 2 mammal 80.5 |
|---|
| 1 mammal NaN |
| We may take elements using negative integers for positive indices. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you clarify that this is for starting at the end?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| >>> df.take([-1, -2]) |
|---|
| name class max_speed |
| 1 monkey mammal NaN |
| 2 lion mammal 80.5 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is a bit unfortunate in the example that it is exactly indices [1, 2] for [-1, -2], so maybe change the indices a bit so they don't match? (also because you said "We may take elements using negative integers for positive indices.", which can be interpreted that negative values are interpreted as the positive index of that value)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| See Also |
| -------- |
| ndarray.take |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need numpy. before it? (not sure, but for clarity maybe does no harm in any case)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| This means that we are not indexing according to actual values in |
| the index attribute of the object. We are indexing according to the |
| actual position of the element in the object's array of values. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just write "actual position of the elements in the object"
("object's array of values" can possibly cause some confusion I think)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect!
Thanks @matagus and @gfyoung !
(btw, @gfyoung I personally find it easier that you add commits instead of amending the previous one, it's sometimes easier to see what has changes (in this case only small diff, so not really a problem, but for larger diffs can be helpful)
@jorisvandenbossche : Absolutely. Good to know.
On a separate note, what is the purpose of the convert parameter (I generally don't use it)? I see no difference in behavior AFAICT (using the docstring example):
df.take([-1, -2]) name class max_speed 1 monkey mammal NaN 3 lion mammal 80.5 df.take([-1, -2], convert=False) name class max_speed 1 monkey mammal NaN 3 lion mammal 80.5
Good catch, reason is very simple, it is not passed through:
| new_data = self._data.take(indices, |
|---|
| axis=self._get_block_manager_axis(axis), |
| convert=True, verify=True) |
But not sure if this should actually be a public parameter
Doh! Yeah, that's a good point. Drilling further down reveals more strangeness:
| if verify: |
|---|
| if ((indexer == -1) | (indexer >= n)).any(): |
| raise Exception('Indices must be nonzero and less than ' |
| 'the axis length') |
The error message doesn't seem to match the check here IINM.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
[ci skip]
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
[ci skip]
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
gfyoung added a commit to forking-repos/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
jreback pushed a commit that referenced this pull request
xref gh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
alanbato pushed a commit to alanbato/pandas that referenced this pull request
alanbato pushed a commit to alanbato/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.
No-Stream pushed a commit to No-Stream/pandas that referenced this pull request
xref pandas-devgh-16948. The parameter is not respected, nor is it a parameter in many 'take' implementations.