pandas (original) (raw)

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 CLN: final Panel cleanup #25632
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

So we will need to keep something like

class Panel:

    def __init__(self, *args, **kwargs):
        raise ImportError("pandas.Panel is removed")

in core/panel.py and still import that in the main pandas namespace to avoid breaking released version of statsmodels and xarray.

Brief glance at the xarray source this is used for introspection so not sure a dummy class is that useful. Perhaps something more cleanly handled downstream? cc @shoyer

statsmodels is doing the same type of checking ?

cc @jbrockmendel

For what it's worth, this is probably an indication that the deprecation notice for Panel was not entirely effective, because it could still be used only for type checks without an error. In the future it would probably make sense to use module level __getattr__ when deprecating classes.

shoyer added a commit to shoyer/xarray that referenced this pull request

Jun 28, 2019

For what it's worth, this is probably an indication that the deprecation notice for Panel was not entirely effective, because it could still be used only for type checks without an error. In the future it would probably make sense to use module level __getattr__ when deprecating classes.

@shoyer well that's very hard with < 3.7 (we did actually have some machinery to do this but blew it away a while back as it was complex) and not worth the effort IMHO.

I would "be nice" for now and don't knowingly break other projects. As @shoyer said, it is hard to catch this as the specific use case does not result in a warning message (so there will probably be other projects as well). If we want to release a RC next week, I think it is better if that RC does not break importing statsmodels or xarray.

Or, @shoyer, do you plan to do a xarray bugfix release soonish?

Keeping a dummy class is not that hard (only a couple of lines of code). For Python 3.7 we could maybe actually use the getattribute trick (if it is possible to do that depending on the python version)

Something like this seems to work (at least on Python 3.7):

diff --git a/pandas/init.py b/pandas/init.py index eafb89c52..d1d4cf678 100644 --- a/pandas/init.py +++ b/pandas/init.py @@ -158,3 +158,18 @@ Here are just a few of the things that pandas does well: conversion, moving window statistics, moving window linear regressions, date shifting and lagging, etc. """ + +if pandas.compat.PY37:

def getattr(name):
```
   if name == 'Panel':
```
```
       import warnings
```
```
       warnings.warn(
```

           "The Panel class is removed from pandas. Accessing it from the "

           "top-level namespace will also be removed in the next version",

```
           FutureWarning, stacklevel=2)
```
```
       return None
```
```
   raise AttributeError(
```

       "module 'pandas' has no attribute {}".format(name))

+else:

class Panel:
```
   pass
```

shoyer added a commit to pydata/xarray that referenced this pull request

Jun 30, 2019

I would prefer not to add something like that since we have been targeting removal on 0.25 for a while and we still then wouldn't be removing it.

To be clear, I don't propose to keep the 1000+ lines of panel code. We are removing the actual Panel class. I only propose to add a few lines to give a more gracious deprecation for a use case we have not been raising a warning for.

I would prefer not to add something like that since we have been targeting removal on 0.25 for a while and we still then wouldn't be removing it.

Agreed. Especially if it is mainly xarray and statsmodels we're worried about, we can help them get quick bugfix releases out and have this over Once And For All.

I think a compatibility shim like #27101 (comment) is good to have for a bit.

@WillAyd do you have time to do that? If not, do you mind if I do it sometime tonight?

Even if xarray issues a bug fix release in the next week (which is quite likely), there are still going to be combinations of pandas with statsmodels/xarray that don't even import. This sort of strict version compatibility requirements is best avoided if possible, and in this case it looks like it would be pretty easy/painless to avoid.

…

yeah i think we need this shim.

Sure will probably get to this tonight

…

Sent from my iPhone

On Jul 2, 2019, at 4:54 PM, Jeff Reback ***@***.***> wrote: yeah i think we need this shim. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

@WillAyd I pushed some compat code; let's see.

@jreback I think we should do the getattr check on Python 3.7 as I showed above in #27101 (comment).
That actually gives a deprecation warning for the use case that we are keeping a dummy class for (only if you are using py 3.7, but that is at least something).

@jreback I think we should do the getattr check on Python 3.7 as I showed above in #27101 (comment).
That actually gives a deprecation warning for the use case that we are keeping a dummy class for (only if you are using py 3.7, but that is at least something).

@jorisvandenbossche ok sure

Hello @WillAyd! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-07-03 15:44:44 UTC

Thanks Jeff! Traveling back west today so feel free to move forward on this without me if a blocker. Otherwise can pick up later this evening

@TomAugspurger I think this needs a conditional in pandas/tests/api/tests_api.py where if < py37 then add to the class list (deprecated), otherwise its not there. I can do later today unless you want to push a patch.

doc/source/whatsnew/v0.15.0.rst(734,6): error E999: SyntaxError: invalid syntax
doc/source/whatsnew/v0.11.0.rst(392,12): error E999: SyntaxError: invalid syntax
Bash exited with code '1'.

"from the top-level namespace will also be removed in "
"the next version",
FutureWarning, stacklevel=2)
return None

Choose a reason for hiding this comment

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

Downstream are expecting this to be a type so they can do isinstance(thing, pd.Panel). Will make another dummy class Panel here (not outside the getattr, so that we don't pollute the namespace on py37+).

@jreback the CI failure is unrelated (the categorical ASV we fixed in the other PR).

Should be good to go.

rs2 mentioned this pull request

Jan 20, 2021

rs2 added a commit to rs2/pandas that referenced this pull request

Jan 21, 2021