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: rename to _consolidate and create deprecation warning #15501

Merged
merged 2 commits into from Feb 28, 2017

Conversation

Projects
None yet
4 participants
@GuessWhoSamFoo
Copy link
Contributor

commented Feb 25, 2017

  • closes #15483
  • tests added / passed
  • passes git diff upstream/master | flake8 --diff
  • whatsnew entry

Test output:

running: pytest --skip-slow --skip-network pandas
============================= test session starts ==============================
platform linux2 -- Python 2.7.13, pytest-3.0.6, py-1.4.32, pluggy-0.4.0
rootdir: /home/sfoo/pandas, inifile: setup.cfg
collected 11882 items / 4 skipped

 10147 passed, 1662 skipped, 77 xfailed, 2084 pytest-warnings in 407.18 seconds  
@jorisvandenbossche

This comment has been minimized.

Copy link
Member

commented Feb 25, 2017

@GuessWhoSamFoo We would like to have it the other way around. Have the deprecation warning in consolidate and not in _consolidate (so this last one can be used internally without warning). So you can leave the existing consolidate intact like it is now, apart from renaming it to _consolidate, and then add a new consolidate that has the deprecation warning and just calls _consolidate (not adding a new _consolidate like you did now).
Further, you will also have to replace internal usage in the pandas codebase of consolidate to _consolidate to avoid raising the deprecation warning (that is the reason for the test failures).

@GuessWhoSamFoo

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2017

==================================== test session starts =====================================
platform linux2 -- Python 2.7.13, pytest-3.0.6, py-1.4.32, pluggy-0.4.0
rootdir: /home/sfoo/pandas, inifile: setup.cfg
collected 11882 items / 4 skipped 

======= 10147 passed, 1662 skipped, 77 xfailed, 2084 pytest-warnings in 419.22 seconds =======

The original consolidate has been left intact except for a line taken out of the docstring. Then I've changed instances of consolidate to _consolidate (even in tests) but still have the same number of failed tests/warnings. Am I missing something here?

@@ -2861,7 +2861,7 @@ def write(self, obj, **kwargs):
super(BlockManagerFixed, self).write(obj, **kwargs)
data = obj._data
if not data.is_consolidated():
data = data.consolidate()
data = data._consolidate()

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Feb 26, 2017

Member

Here, data is not a DataFrame, but a BlockManager, so this should be left as consolidate (that's the reason for the test failures)

@jsexauer jsexauer referenced this pull request Feb 26, 2017

Open

DEPR: deprecations from prior versions #6581

0 of 94 tasks complete

@jreback jreback added the Deprecate label Feb 26, 2017

@@ -484,6 +484,7 @@ Deprecations
- ``DataFrame.astype()`` has deprecated the ``raise_on_error`` parameter in favor of ``errors`` (:issue:`14878`)
- ``Series.sortlevel`` and ``DataFrame.sortlevel`` have been deprecated in favor of ``Series.sort_index`` and ``DataFrame.sort_index`` (:issue:`15099`)
- importing ``concat`` from ``pandas.tools.merge`` has been deprecated in favor of imports from the ``pandas`` namespace. This should only affect explict imports (:issue:`15358`)
- ``DataFrame.consolidate()`` has been deprecated. (:issue:`15483`)

This comment has been minimized.

Copy link
@jreback

jreback Feb 27, 2017

Contributor

say Series/DataFrame/Panel.consolidate() it affects everything

say has been deprecated as a public method as

"""
DEPRECATED:

This comment has been minimized.

Copy link
@jreback

jreback Feb 27, 2017

Contributor

this is not deprecated though.

can you add a doc-string

@@ -2897,6 +2898,12 @@ def consolidate(self, inplace=False):
cons_data = self._protect_consolidate(f)
return self._constructor(cons_data).__finalize__(self)

def consolidate(self, inplace=False):
# 15483

This comment has been minimized.

Copy link
@jreback

jreback Feb 27, 2017

Contributor

here is where the DEPRECATED goes (in the doc-string)

@@ -40,17 +40,17 @@ def test_cast_internals(self):

def test_consolidate(self):

This comment has been minimized.

Copy link
@jreback

jreback Feb 27, 2017

Contributor

can you add a single test (test_consolidate_deprecation), which uses assert_produces_warning(FutureWarning)

def consolidate(self, inplace=False):
# 15483
warnings.warn("consolidate is deprecated and will be removed in a "
"future release.", DeprecationWarning)

This comment has been minimized.

Copy link
@jreback

jreback Feb 27, 2017

Contributor

use FutureWarning

DEPR: rename to _consolidate and create deprecation warning
Changed internal references to consolidate and updated tests

Updated per review comments

@GuessWhoSamFoo GuessWhoSamFoo force-pushed the GuessWhoSamFoo:dep-consolidate branch from ee3ec7a to 4e927e6 Feb 27, 2017

@GuessWhoSamFoo

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2017

Squashed the last two commits with the changes from the feedback. Thank you for the guidance!

@codecov-io

This comment has been minimized.

Copy link

commented Feb 28, 2017

Codecov Report

Merging #15501 into master will increase coverage by 0.71%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #15501      +/-   ##
==========================================
+ Coverage   90.36%   91.08%   +0.71%     
==========================================
  Files         136      136              
  Lines       49552    49102     -450     
==========================================
- Hits        44780    44725      -55     
+ Misses       4772     4377     -395
Impacted Files Coverage Δ
pandas/core/groupby.py 95% <100%> (+0.01%)
pandas/tseries/resample.py 94.47% <100%> (ø)
pandas/core/generic.py 96.31% <100%> (ø)
pandas/tools/concat.py 97.62% <100%> (ø)
pandas/io/pytables.py 93.9% <100%> (ø)
pandas/util/decorators.py 67.32% <0%> (-1.28%)
pandas/core/frame.py 97.73% <0%> (-0.1%)
pandas/util/print_versions.py 15.71% <0%> (ø)
pandas/types/cast.py 85.65% <0%> (+0.2%)
... and 1 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 e0647ba...c838da8. Read the comment docs.

@jorisvandenbossche jorisvandenbossche merged commit 7b84eb6 into pandas-dev:master Feb 28, 2017

0 of 3 checks passed

ci/circleci CircleCI is running your tests
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@jorisvandenbossche

This comment has been minimized.

Copy link
Member

commented Feb 28, 2017

@GuessWhoSamFoo Thank you!

@GuessWhoSamFoo GuessWhoSamFoo deleted the GuessWhoSamFoo:dep-consolidate branch Mar 5, 2017

AnkurDedania added a commit to AnkurDedania/pandas that referenced this pull request Mar 21, 2017

gfyoung added a commit to forking-repos/pandas that referenced this pull request Oct 27, 2018

@jreback jreback referenced this pull request Oct 27, 2018

Open

DEPR: deprecations log for removed issues #13777

135 of 135 tasks complete

jreback added a commit that referenced this pull request Oct 28, 2018

brute4s99 added a commit to brute4s99/pandas that referenced this pull request Nov 19, 2018

Pingviinituutti added a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

Pingviinituutti added 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
You can’t perform that action at this time.