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

Standardize function signatures #14645

Merged

Conversation

gfyoung
Copy link
Member

@gfyoung gfyoung commented Nov 13, 2016

Standardize the following function signatures:

  1. repeat(reps, *args, **kwargs)
  2. searchsorted(value, side='left', sorter=None)

Closes #12662.

@codecov-io
Copy link

codecov-io commented Nov 13, 2016

Current coverage is 85.22% (diff: 97.67%)

Merging #14645 into master will increase coverage by <.01%

@@             master     #14645   diff @@
==========================================
  Files           143        143          
  Lines         50807      50813     +6   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          43297      43304     +7   
+ Misses         7510       7509     -1   
  Partials          0          0          

Powered by Codecov. Last update d8e427b...f448639

@@ -1818,3 +1818,31 @@ def test_op_duplicate_index(self):
result = s1 + s2
expected = pd.Series([11, 12, np.nan], index=[1, 1, 2])
assert_series_equal(result, expected)

def test_searchsorted(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

put these tests alongside the other ones / u need to search for them

Copy link
Member Author

Choose a reason for hiding this comment

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

Alongside what other ones? Where should I put these tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

u need to search for them
i think that's pretty clear

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough, though as written, the "/" made the sentence confusing.

@@ -53,6 +53,9 @@ Other API Changes
Deprecations
^^^^^^^^^^^^

- ``Series.repeat()`` has deprecated the ``reps`` parameter in favor of ``repeats`` (:issue:`12662`)
- ``Index.repeat()`` and ``MultiIndex.repeat()`` have deprecated the ``n`` parameter in favor of ``repeats`` (:issue:`12662`)
- ``Categorical.searchsorted()`` and ``Series.searchsorted()`` have deprecated the ``v`` parameter in favor of ``key`` (:issue:`12662`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't numpy still call this parameter v?

In [9]: a = np.array([1,2,3])
In [11]: a.searchsorted?
Docstring:
a.searchsorted(v, side='left', sorter=None)

Find indices where elements of v should be inserted in a to maintain order.

For full documentation, see `numpy.searchsorted`

See Also
--------
numpy.searchsorted : equivalent function
Type:      builtin_function_or_method

In [12]: np.__version__
Out[12]: '1.11.1'

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see that Index was already using key as the name - I guess I don't feel strongly between the two.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see a reason to follow the numpy convention, especially since it is very unclear what exactly that parameter is. Unless someone strongly objects, I would prefer to use key.

Copy link
Contributor

Choose a reason for hiding this comment

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

so we typically will use value in this case (not sure why it was key in the timedelta one). We are searching the .values of the object, and NOT the index (where we typically use key).

Copy link
Member Author

@gfyoung gfyoung Nov 23, 2016

Choose a reason for hiding this comment

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

@jreback : Okay...so should we still keep to key or should they all be changed to value for searchsorted?

Copy link
Member

Choose a reason for hiding this comment

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

Since we are already changing keyword names, for me it is OK to choose value instead of key

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, fair enough. Will change.

@gfyoung
Copy link
Member Author

gfyoung commented Nov 13, 2016

@jreback , @chris-b1 : Everything looks green. Ready to merge if there are no concerns.

@jreback
Copy link
Contributor

jreback commented Nov 15, 2016

lgtm.

@gfyoung gfyoung force-pushed the signature-standardize branch 9 times, most recently from c4e0bb8 to 26f7be9 Compare November 22, 2016 18:28
tm.assert_index_equal(result, expected)

with tm.assert_produces_warning(FutureWarning,
check_stacklevel=False):
Copy link
Member

Choose a reason for hiding this comment

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

Is the check_stacklevel=False needed here? This looks like a simple example where stacklevel=2 should just work

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is needed unfortunately.

Copy link
Member

Choose a reason for hiding this comment

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

For which builds does it error? If we always have to pass check_stacklevel=False, even for those (I think) straightforward case, there is not much point in having the keyword.

For me it passes locally.

Copy link
Member Author

@gfyoung gfyoung Nov 25, 2016

Choose a reason for hiding this comment

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

It didn't pass locally for me. TBH, not really sure why we need this argument in the first place.

Copy link
Member

Choose a reason for hiding this comment

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

Can you show the error traceback?

The reason we have this argument, is to ensure that the FutureWarnings are correctly attributed. Towards users, this is really helpful.

Copy link
Member

Choose a reason for hiding this comment

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

See #9584

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps, but look at how many times we have had to pass in check_stacklevel=False. I don't necessarily object to specifying a stack level, but if we're unable to consistently specify the correct stack level, why are we using it in our tests?

BTW, I'm not trying to avoid showing my error traceback. I'm just in the middle of another PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ran the tests again to see if I could replicate the issue but invoked the tests in a different way, and I could not replicate. So let's see what Travis says about it.

@gfyoung gfyoung force-pushed the signature-standardize branch 3 times, most recently from 3d469de to 3cdded5 Compare November 25, 2016 09:39
@gfyoung
Copy link
Member Author

gfyoung commented Nov 25, 2016

@jreback , @jorisvandenbossche : Everything seems to be passing still. Ready to merge if there are no other concerns.

- ``Series.repeat()`` has deprecated the ``reps`` parameter in favor of ``repeats`` (:issue:`12662`)
- ``Index.repeat()`` and ``MultiIndex.repeat()`` have deprecated the ``n`` parameter in favor of ``repeats`` (:issue:`12662`)
- ``Categorical.searchsorted()`` and ``Series.searchsorted()`` have deprecated the ``v`` parameter in favor of ``value`` (:issue:`12662`)
- ``IndexOpsMixin.searchsorted()``, ``TimedeltaIndex.searchsorted()``, ``DatetimeIndex.searchsorted()``, and ``PeriodIndex.searchsorted()`` have deprecated the ``key`` parameter in favor of ``value`` (:issue:`12662`)
Copy link
Member

Choose a reason for hiding this comment

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

Listing IndexOpsMixin is not needed here I think

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough. Done.

@@ -97,6 +97,10 @@ def test_repeat(self):
numbers, names.repeat(reps)], names=names)
tm.assert_index_equal(m.repeat(reps), expected)

with tm.assert_produces_warning(FutureWarning,
check_stacklevel=False):
Copy link
Member

Choose a reason for hiding this comment

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

I think you can removed all other occurences of check_stacklevel=False in this PR as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems so. Done.

Standardize the following function signatures:

1) repeat(reps, *args, **kwargs)
2) searchsorted(value, side='left', sorter=None)

Closes pandas-devgh-12662.
@gfyoung
Copy link
Member Author

gfyoung commented Nov 26, 2016

@jreback , @jorisvandenbossche : Changes made, and everything seems to be passing still. Ready to merge if there are no other concerns.

@jorisvandenbossche jorisvandenbossche merged commit 08d7b2c into pandas-dev:master Nov 26, 2016
@gfyoung gfyoung deleted the signature-standardize branch November 26, 2016 09:31
gfyoung added a commit to forking-repos/pandas that referenced this pull request Aug 19, 2018
jreback pushed a commit that referenced this pull request Aug 20, 2018
gfyoung added a commit to forking-repos/pandas that referenced this pull request Sep 2, 2018
gfyoung added a commit to forking-repos/pandas that referenced this pull request Sep 2, 2018
jreback pushed a commit that referenced this pull request Sep 4, 2018
gfyoung added a commit to forking-repos/pandas that referenced this pull request Sep 11, 2018
The parameter is "value" across the board.

xref pandas-devgh-14645.
gfyoung added a commit to forking-repos/pandas that referenced this pull request Sep 11, 2018
The parameter is "value" across the board.

xref pandas-devgh-14645.
gfyoung added a commit to forking-repos/pandas that referenced this pull request Sep 11, 2018
The parameter is "value" across the board.

xref pandas-devgh-14645.
gfyoung added a commit to forking-repos/pandas that referenced this pull request Sep 12, 2018
"values" is the law of the land.

xref pandas-devgh-14645.
gfyoung added a commit to forking-repos/pandas that referenced this pull request Sep 12, 2018
"values" is the law of the land.

xref pandas-devgh-14645.

Follow-up to pandas-devgh-15601.
gfyoung added a commit to forking-repos/pandas that referenced this pull request Sep 13, 2018
"values" is the law of the land. This usage
is internal, hence why we aren't going
through a deprecation cycle.

xref pandas-devgh-14645.

Follow-up to pandas-devgh-15601.
gfyoung added a commit to forking-repos/pandas that referenced this pull request Sep 13, 2018
"values" is the law of the land. This usage
is internal, hence why we aren't going
through a deprecation cycle.

xref pandas-devgh-14645.

Follow-up to pandas-devgh-15601.
gfyoung added a commit that referenced this pull request Sep 13, 2018
The parameter is "value" across the board.

xref gh-14645.
gfyoung added a commit to forking-repos/pandas that referenced this pull request Sep 13, 2018
"values" is the law of the land. This usage
is internal, hence why we aren't going
through a deprecation cycle.

xref pandas-devgh-14645.

Follow-up to pandas-devgh-15601.
gfyoung added a commit to forking-repos/pandas that referenced this pull request Sep 14, 2018
"values" is the law of the land.

xref pandas-devgh-14645.

Follow-up to pandas-devgh-15601.
gfyoung added a commit that referenced this pull request Sep 15, 2018
"value" is the law of the land.

xref gh-14645.

Follow-up to gh-15601.
aeltanawy pushed a commit to aeltanawy/pandas that referenced this pull request Sep 20, 2018
aeltanawy pushed a commit to aeltanawy/pandas that referenced this pull request Sep 20, 2018
aeltanawy pushed a commit to aeltanawy/pandas that referenced this pull request Sep 20, 2018
Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018
Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018
Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018
Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 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