DEPR: rename to _consolidate and create deprecation warning by GuessWhoSamFoo · Pull Request #15501 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation11 Commits2 Checks0 Files changed
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 DEPR: .consolidate() should be non-public #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
@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).
==================================== 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() |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, data is not a DataFrame, but a BlockManager, so this should be left as consolidate (that's the reason for the test failures)
| @@ -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`) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
say Series/DataFrame/Panel.consolidate() it affects everything
say has been deprecated as a public method as
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here is where the DEPRECATED goes (in the doc-string)
| @@ -40,17 +40,17 @@ def test_cast_internals(self): |
|---|
| def test_consolidate(self): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use FutureWarning
Changed internal references to consolidate and updated tests
Updated per review comments
Squashed the last two commits with the changes from the feedback. Thank you for the guidance!
AnkurDedania pushed a commit to AnkurDedania/pandas that referenced this pull request
gfyoung added a commit to forking-repos/pandas that referenced this pull request
jreback pushed a commit that referenced this pull request
tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request