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: remove pandas.core.common is_* #19769

Merged
merged 3 commits into from Feb 22, 2018
Merged

Conversation

jreback
Copy link
Contributor

@jreback jreback commented Feb 19, 2018

xref #13990
xref #19304

@jreback jreback added the Deprecate Functionality to remove in pandas label Feb 19, 2018
@jreback jreback added this to the 0.23.0 milestone Feb 19, 2018
Copy link
Contributor

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

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

Sorry I forgot about #19304

@@ -624,6 +624,7 @@ Removal of prior version deprecations/changes
- The modules `pandas.tools.hashing` and `pandas.util.hashing` have been removed (:issue:`16223`)
- The top-level functions ``pd.rolling_*``, ``pd.expanding_*`` and ``pd.ewm*`` have been removed (Deprecated since v0.18).
Instead, use the DataFrame/Series methods :attr:`~DataFrame.rolling`, :attr:`~DataFrame.expanding` and :attr:`~DataFrame.ewm` (:issue:`18723`)
- Imports from ``pandas.core.common`` for api like functions such as ``is_datetime64_dtype`` are now removed. These are located in ``pandas.api.types``. Furthermore, errors are removed as well from ``pandas.core.common`` and are available in ``pandas.errors`` (:issue:`13990`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can remove "for api" and just have "functions like is_datetime64_dtype."

And using the errors here didn't actually emit deprecation warnings did it? Those should be deprecated before removing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, we simply have to remove them. these are classes and we were never able to actually deprecate them.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can deprecate pretty easily with subclassing

class PerformanceWarning(pandas.errors.PerformanceWarning):
    def __init__(self, *args, **kwargs):
        warnings.warn('Deprecated. Use pandas.errors instead', FutureWarning)
        super().__init__(*args, **kwargs)

could meta-program that if you want, but it's just 4 errors.

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 believe @jorisvandenbossche tried this, and we had a discussion about this in the original issue.

Copy link
Member

Choose a reason for hiding this comment

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

The problem we discussed AFAIR was about the problem people importing an error to then only catch it. That way the error is never 'called', and a deprecation warning like the above would not work.
It does work for raising such a warning (but I think catching will be more common).

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 the only way this can really be solved is by renaming core/common.py to eg core/utils.py, and then you can deprecate the whole core/common.py module, and already raise warning on importing it.

Copy link
Member

Choose a reason for hiding this comment

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

Could override isinstancecheck? Probably overkill.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I think we had this discussion a while back, and it looks like it can be done, but then the details kill us. @jorisvandenbossche soln would work, but I think will just put a big warning.

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Feb 19, 2018 via email

@codecov
Copy link

codecov bot commented Feb 19, 2018

Codecov Report

Merging #19769 into master will decrease coverage by 0.04%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #19769      +/-   ##
==========================================
- Coverage   91.61%   91.57%   -0.05%     
==========================================
  Files         150      150              
  Lines       48887    48865      -22     
==========================================
- Hits        44786    44746      -40     
- Misses       4101     4119      +18
Flag Coverage Δ
#multiple 89.94% <50%> (-0.05%) ⬇️
#single 41.79% <16.66%> (-0.02%) ⬇️
Impacted Files Coverage Δ
pandas/core/common.py 92.9% <ø> (-0.52%) ⬇️
pandas/core/base.py 96.8% <40%> (ø) ⬆️
pandas/core/resample.py 96.43% <57.14%> (ø) ⬆️
pandas/plotting/_converter.py 65.22% <0%> (-1.74%) ⬇️
pandas/core/dtypes/common.py 93.91% <0%> (-1.46%) ⬇️

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 e022d9a...56017ca. Read the comment docs.

@jreback jreback merged commit 842d350 into pandas-dev:master Feb 22, 2018
harisbal pushed a commit to harisbal/pandas that referenced this pull request Feb 28, 2018
gfyoung added a commit to forking-repos/pandas that referenced this pull request Dec 31, 2018
gfyoung added a commit that referenced this pull request Dec 31, 2018
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants