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

DEPR: Deprecate NDFrame.swapaxes #26654

Closed
wants to merge 3 commits into from

Conversation

topper-123
Copy link
Contributor

.swapaxes are not needed, now that Panel is being removed.

@topper-123 topper-123 added the Deprecate Functionality to remove in pandas label Jun 5, 2019
@topper-123 topper-123 force-pushed the deprecate_swapaxes branch 2 times, most recently from 6d0f791 to 7e6bd88 Compare June 5, 2019 09:19
@topper-123 topper-123 added the Reshaping Concat, Merge/Join, Stack/Unstack, Explode label Jun 5, 2019
@topper-123 topper-123 added this to the 0.25.0 milestone Jun 5, 2019
@codecov
Copy link

codecov bot commented Jun 5, 2019

Codecov Report

Merging #26654 into master will decrease coverage by 50.06%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #26654       +/-   ##
===========================================
- Coverage   91.87%   41.81%   -50.07%     
===========================================
  Files         174      174               
  Lines       50683    50684        +1     
===========================================
- Hits        46567    21191    -25376     
- Misses       4116    29493    +25377
Flag Coverage Δ
#multiple ?
#single 41.81% <0%> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/core/groupby/ops.py 19.76% <0%> (-76.24%) ⬇️
pandas/core/generic.py 37.96% <0%> (-55.67%) ⬇️
pandas/io/formats/latex.py 0% <0%> (-100%) ⬇️
pandas/io/sas/sas_constants.py 0% <0%> (-100%) ⬇️
pandas/core/groupby/categorical.py 0% <0%> (-100%) ⬇️
pandas/tseries/plotting.py 0% <0%> (-100%) ⬇️
pandas/tseries/converter.py 0% <0%> (-100%) ⬇️
pandas/io/formats/html.py 0% <0%> (-99.37%) ⬇️
pandas/io/sas/sas7bdat.py 0% <0%> (-91.16%) ⬇️
pandas/io/sas/sas_xport.py 0% <0%> (-90.1%) ⬇️
... and 130 more

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 01d97d4...676a2e4. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 5, 2019

Codecov Report

Merging #26654 into master will increase coverage by 50.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #26654       +/-   ##
===========================================
+ Coverage   41.84%   91.86%   +50.01%     
===========================================
  Files         174      174               
  Lines       50663    50664        +1     
===========================================
+ Hits        21201    46543    +25342     
+ Misses      29462     4121    -25341
Flag Coverage Δ
#multiple 90.4% <100%> (?)
#single 41.75% <0%> (-0.1%) ⬇️
Impacted Files Coverage Δ
pandas/core/internals/blocks.py 94.12% <ø> (+41.36%) ⬆️
pandas/core/generic.py 93.54% <100%> (+55.55%) ⬆️
pandas/core/groupby/ops.py 96% <100%> (+76.23%) ⬆️
pandas/core/computation/pytables.py 90.24% <0%> (+0.3%) ⬆️
pandas/io/pytables.py 90.29% <0%> (+0.96%) ⬆️
pandas/core/panel.py 17.61% <0%> (+1.68%) ⬆️
pandas/util/_test_decorators.py 93.22% <0%> (+5.08%) ⬆️
pandas/io/gbq.py 78.94% <0%> (+5.26%) ⬆️
pandas/_config/localization.py 86% <0%> (+6%) ⬆️
pandas/core/config_init.py 96.85% <0%> (+10.23%) ⬆️
... and 131 more

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 1d7ad5f...518ef5d. Read the comment docs.

@@ -476,6 +476,8 @@ Other Deprecations
the :meth:`SparseArray.to_dense` method instead (:issue:`26421`).
- The functions :func:`pandas.to_datetime` and :func:`pandas.to_timedelta` have deprecated the ``box`` keyword. Instead, use :meth:`to_numpy` or :meth:`Timestamp.to_datetime64` or :meth:`Timedelta.to_timedelta64`. (:issue:`24416`)
- The :meth:`DataFrame.compound` and :meth:`Series.compound` methods are deprecated and will be removed in a future version (:issue:`26405`).
- The :meth:`DataFrame.swapaxes` and :meth:`Series.swapaxes` methods are deprecated and will be removed in a future version.
Use :meth:`DataFrame.transpose` and :meth:`Series.transpose` instead(:issue:`26654`).
Copy link
Contributor

Choose a reason for hiding this comment

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

space before the issue number

@topper-123
Copy link
Contributor Author

This passes locally, but is somehow failing on numpy_dev. I'll see it I can figure it out.

with pytest.raises(ValueError, match=msg):
df.swapaxes(2, 5)

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 there a reason you need the check_stacklevel=False ? (it seems a simple first level function, not multiple levels deep)

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 got an error related to stack level, because something else is also deprecated. I can't exactly remember which function was deprecated, but I just thought this was the easy solution (the exact stack level isn't that important...)

Copy link
Member

Choose a reason for hiding this comment

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

(the exact stack level isn't that important...)

It is relevant for the user experience. But the stacklevel of 2 should be correct.
But a different deprecation being raised sounds a bit suspicious. Can you check which one it was?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has something to do with SparseDataFrame:

    def test_swapaxes_deprecated(self):
        df = self.klass(np.random.randn(10, 5))

        with tm.assert_produces_warning(FutureWarning):
>           swapped = df.swapaxes(0, 1)

pandas\tests\frame\test_api.py:370:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <contextlib._GeneratorContextManager object at 0x000002453AD9E940>, type = None, value = None, traceback = None

    def __exit__(self, type, value, traceback):
        if type is None:
            try:
>               next(self.gen)
E               AssertionError: Warning not set with correct stacklevel. File where warning is raised: C:\Users\TP\Documents\Python\pandasdev\pandasdev\pandas\core\generic.py != C:\Users\TP\Documents\Python\pandasdev\pandasdev\pandas\tests\frame\test_api.py. Warning message: SparseDataFrame is deprecated and will be removed in a future version.
E               Use a regular DataFrame whose columns are SparseArrays instead.
E
E               See http://pandas.pydata.org/pandas-docs/stable/user_guide/sparse.html#migrating for more.

I'll try to work around it.

Copy link
Contributor

Choose a reason for hiding this comment

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

assert_produces_warning doesn't handle stack level correctly with multiple warnings. I'd recommend just check_stacklevel=False like you've done.

Copy link
Member

Choose a reason for hiding this comment

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

or don't test it for SparseDataFrame? That's deprecated anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you explain why the stack level important? Seems not so relevant to me.

Copy link
Member

Choose a reason for hiding this comment

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

It's not that important here, as I can see you set it correctly.

But, in general, we want to test it because it ensures that end-users get a warning with the correction "location" indicated. If the stacklevel is set correctly, the warning message points to where this deprecated feature is being used. If not, it points to some other place, which is not useful or even plain confusing.

@jorisvandenbossche
Copy link
Member

This passes locally, but is somehow failing on numpy_dev. I'll see it I can figure it out.

From the tracebacks on Azure, it might be from np.swapaxes which ends up calling the swapaxes method, raising the warning, and warnings from numpy are turned into errors on that build I think.

Not sure there is anything we can do about that .. (apart from actually implementing __array_function__)

@topper-123
Copy link
Contributor Author

topper-123 commented Jun 5, 2019

From the tracebacks on Azure, it might be from np.swapaxes which ends up calling the swapaxes method, raising the warning, and warnings from numpy are turned into errors on that build I think.

Thanks, that's something to investigate. Could be there's a np.swapaxes(df) that could be changed to np.swapaxes(df.values) somewhere?

@jorisvandenbossche
Copy link
Member

Could be there's a np.swapaxes(df) that could be changed to np.swapaxes(df.values) somewhere?

I suppose it is actually numpy calling it. Basically, np.swapaxes(obj, ..) will check if obj has a swapaxes method. That also happens with mean, sum, etc, so I suppose that might be the case here as wel.

@@ -431,7 +431,7 @@ def test_size_compat(self):

def test_split_compat(self):
# xref GH8846
o = self._construct(shape=10)
o = self._construct(shape=10).values
assert len(np.array_split(o, 5)) == 5
Copy link
Contributor Author

Choose a reason for hiding this comment

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

nparray_split depends on np.swapaxes, so I sidestep the problem here. This test is not very useful. Is that __array_function__ in the pipeline to implement?

I'm thinking that maybe we should maybe keep swapaxes, perhaps hidden in _deprecations like we do with tolist?

@@ -431,7 +431,7 @@ def test_size_compat(self):

def test_split_compat(self):
# xref GH8846
o = self._construct(shape=10)
o = self._construct(shape=10).values
Copy link
Member

Choose a reason for hiding this comment

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

This defeats the purpose of the test, I think, as it was actually testing that np.array_split works on a DataFrame/Series.

So the "quick fix" is rather to ignore warnings in this specific test, so it doesn't fail the test, I think.

Ideally we would prevent that the numpy function raises a warning at all, but not sure that is easily possible.

@topper-123
Copy link
Contributor Author

I'm taking this private for a while. I think np.swapaxes(df, 0, 1) should work on principle, and I'll see if I can get something to work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecate Functionality to remove in pandas Reshaping Concat, Merge/Join, Stack/Unstack, Explode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants