Deprecate non-keyword arguments in mask #41580

ShreyDixit · 2021-05-20T07:46:31Z

xref Deprecate non-keyword arguments for methods with inplace #41485
tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry

MarcoGorelli

This is off to a good start! Just a few comments, and some pre-commit checks to fixup

MarcoGorelli · 2021-05-23T17:32:32Z

pandas/core/generic.py

        name="mask",
        name_other="where",
    )
+    @deprecate_nonkeyword_arguments(version="2.0", allowed_args=["self", "cond"])


let's make this version=None

also, let's allow other to be positional as well, just like in where https://github.com/pandas-dev/pandas/pull/41523/files

MarcoGorelli · 2021-05-23T17:34:52Z

pandas/tests/frame/indexing/test_mask.py

+        with tm.assert_produces_warning(FutureWarning, match=msg):
+            tm.assert_frame_equal(df.mask(cond, other), df.mask(cond, other=other))


can you write this as

with tm.assert_produces_warnnig... result = ... expected = ... tm.assert_frame_equal(result, expected)

and construct expected explicitly

MarcoGorelli · 2021-05-23T17:35:06Z

pandas/tests/series/indexing/test_mask.py

+    with tm.assert_produces_warning(FutureWarning, match=msg):
+        tm.assert_series_equal(s.mask(cond, np.nan), s.mask(cond, other=np.nan))


same comments as for the DataFrame test

MarcoGorelli · 2021-05-23T17:35:38Z

pandas/tests/frame/indexing/test_mask.py

+        msg = (
+            r"Starting with Pandas version 2\.0 all arguments of mask except for the "
+            r"arguments 'self' and 'cond' will be keyword-only"
+        )


the message will need updating, there's been some changes in deprecate_nonkeyword_arguments

Have made the changes and removed the message entirely.

…ixit/pandas

MarcoGorelli

Generally looks good, just a few comments - also, please remove path_to_file.xlsx

MarcoGorelli · 2021-05-24T16:45:01Z

pandas/tests/frame/indexing/test_mask.py


+    def test_mask_pos_args_deprecation(self):
+        # https://github.com/pandas-dev/pandas/issues/41485
+        df = DataFrame(np.random.randn(5, 5))


could you construct it explicitly instead of passing in randomly generated numbers? a small dataframe with a single column should be fine

MarcoGorelli · 2021-05-24T16:45:30Z

pandas/tests/frame/indexing/test_mask.py

+        with tm.assert_produces_warning(FutureWarning):
+            result = df.mask(cond, other, False)
+
+        expected = df.mask(cond, other=other, inplace=False)


can you construct expected explicitly (otherwise both result and expected go through .mask)

MarcoGorelli · 2021-05-24T16:45:49Z

pandas/tests/frame/indexing/test_mask.py

+        cond = df > 0
+        other = DataFrame(np.random.randn(5, 3))
+
+        with tm.assert_produces_warning(FutureWarning):


can you check the warning message? see msg = ... from the example PR

MarcoGorelli · 2021-05-24T16:46:05Z

pandas/tests/series/indexing/test_mask.py

    tm.assert_series_equal(result, expected)
+
+
+def test_mask_pos_args_deprecation():


same comments as for the DataFrame test

jreback · 2021-05-24T18:23:32Z

hmm other is prob ok as a positional as well here

MarcoGorelli · 2021-05-24T18:25:13Z

hmm other is prob ok as a positional as well here

OK, thanks for stepping in - I'll ask for it to be positional in the where PR too then

…recation

MarcoGorelli · 2021-05-25T19:41:29Z

pandas/tests/frame/indexing/test_mask.py

+        cond = df % 2 == 0
+
+        msg = (
+            r"In a future version of pandas all arguments of NDFrame.mask except for "


should be DataFrame.mask

MarcoGorelli · 2021-05-25T19:41:46Z

pandas/tests/series/indexing/test_mask.py

+    cond = s % 2 == 0
+
+    msg = (
+        r"In a future version of pandas all arguments of NDFrame.mask except for "


Series.mask

MarcoGorelli · 2021-05-25T19:41:59Z

doc/source/whatsnew/v1.3.0.rst

 - Deprecated setting :attr:`Categorical._codes`, create a new :class:`Categorical` with the desired codes instead (:issue:`40606`)
 - Deprecated behavior of :meth:`DatetimeIndex.union` with mixed timezones; in a future version both will be cast to UTC instead of object dtype (:issue:`39328`)
 - Deprecated using ``usecols`` with out of bounds indices for ``read_csv`` with ``engine="c"`` (:issue:`25623`)
+- Deprecated passing arguments (apart from ``cond`` and ``other``) as positional in :meth:`DataFrame.mask` (:issue:`41485`)


mention Series.mask as well

…k_pos_args_deprecation

jreback · 2021-05-27T01:14:18Z

@ShreyDixit can u rebase

…recation

simonjayhawkins

Thanks @ShreyDixit lgtm pending green

jreback · 2021-05-27T14:43:56Z

thanks @ShreyDixit

ShreyDixit · 2021-05-27T15:36:06Z

Thank you everyone for you guidance

ShreyDixit added 4 commits May 19, 2021 10:53

passed series mask tests

06dcad6

Updated stack level for try_cast deprecate warning

6963374

check equality of frames and series

e406b0f

rechanged dockerfile

6e72701

simonjayhawkins added the Deprecate Functionality to remove in pandas label May 21, 2021

MarcoGorelli self-requested a review May 23, 2021 17:31

MarcoGorelli suggested changes May 23, 2021

View reviewed changes

added other to allowed pos args in mask

f7be5e0

ShreyDixit force-pushed the mask_pos_args_deprecation branch from 56e1757 to f7be5e0 Compare May 24, 2021 14:36

ShreyDixit and others added 3 commits May 24, 2021 14:45

Merge remote-tracking branch 'upstream/master'

106ca0c

Merge branch 'master' into mask_pos_args_deprecation

154135b

Merge branch 'mask_pos_args_deprecation' of https://github.com/ShreyD…

fcc9b70

…ixit/pandas

MarcoGorelli suggested changes May 24, 2021

View reviewed changes

simonjayhawkins added this to the 1.3 milestone May 24, 2021

Jiezheng2018 mentioned this pull request May 24, 2021

added deprecate_nonkeyword_arguments to function where #41523

Merged

ShreyDixit added 2 commits May 25, 2021 12:45

Merge remote-tracking branch 'upstream/master' into mask_pos_args_dep…

383202b

…recation

Added message to mask deprec test

4840f66

MarcoGorelli suggested changes May 25, 2021

View reviewed changes

Merge branch 'master' into mask_pos_args_deprecation

4a3e82b

jreback mentioned this pull request May 26, 2021

DEPR: log of deprecations in 1.x (to be removed in 2.0) #30228

Closed

MarcoGorelli added 3 commits May 26, 2021 21:18

Merge remote-tracking branch 'upstream/master' into pr/ShreyDixit-mas…

9f7adc6

…k_pos_args_deprecation

fixup

7861647

fixup

31c98c1

MarcoGorelli force-pushed the mask_pos_args_deprecation branch from 01881bb to 31c98c1 Compare May 26, 2021 20:26

ShreyDixit and others added 3 commits May 27, 2021 05:19

changed whats new

698389a

Merge remote-tracking branch 'upstream/master' into mask_pos_args_dep…

3480092

…recation

Merge branch 'master' into mask_pos_args_deprecation

7b25587

simonjayhawkins approved these changes May 27, 2021

View reviewed changes

jreback merged commit beabf0a into pandas-dev:master May 27, 2021

ShreyDixit deleted the mask_pos_args_deprecation branch May 27, 2021 15:35

TLouf pushed a commit to TLouf/pandas that referenced this pull request Jun 1, 2021

Deprecate non-keyword arguments in mask (pandas-dev#41580)

281dcc3

JulianWgs pushed a commit to JulianWgs/pandas that referenced this pull request Jul 3, 2021

Deprecate non-keyword arguments in mask (pandas-dev#41580)

781d9e9

		with tm.assert_produces_warning(FutureWarning, match=msg):
		tm.assert_frame_equal(df.mask(cond, other), df.mask(cond, other=other))

		with tm.assert_produces_warning(FutureWarning, match=msg):
		tm.assert_series_equal(s.mask(cond, np.nan), s.mask(cond, other=np.nan))

		tm.assert_series_equal(result, expected)


		def test_mask_pos_args_deprecation():

Uh oh!

Deprecate non-keyword arguments in mask #41580

Deprecate non-keyword arguments in mask #41580

Uh oh!

Conversation

ShreyDixit commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarcoGorelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoGorelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreback commented May 24, 2021

Uh oh!

MarcoGorelli commented May 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreback commented May 27, 2021

Uh oh!

simonjayhawkins left a comment

Choose a reason for hiding this comment

Uh oh!

jreback commented May 27, 2021

Uh oh!

ShreyDixit commented May 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ShreyDixit commented May 20, 2021 •

edited

Loading