DEPR: Deprecate Series/Dataframe.to_dense/to_sparse #26684

VikramjeetD · 2019-06-06T03:49:53Z

closes DEPR: deprecate Series/DataFrame.to_dense/to_sparse #26557
passes git diff upstream/master -u -- "*.py" | flake8 --diff
add whatsnew entry
add tests

Marks the methods mentioned in #26557 as deprecated, with proper messages.

codecov · 2019-06-06T04:26:45Z

Codecov Report

Merging #26684 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #26684      +/-   ##
==========================================
+ Coverage   91.87%    91.9%   +0.02%     
==========================================
  Files         174      174              
  Lines       50661    50916     +255     
==========================================
+ Hits        46547    46796     +249     
- Misses       4114     4120       +6

Flag	Coverage Δ
#multiple	`90.45% <100%> (+0.04%)`	⬆️
#single	`41.79% <36.36%> (-0.08%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/sparse/frame.py	`96.59% <100%> (+0.96%)`	⬆️
pandas/core/frame.py	`97% <100%> (-0.12%)`	⬇️
pandas/core/series.py	`93.62% <100%> (ø)`	⬆️
pandas/core/sparse/series.py	`95.75% <100%> (+0.85%)`	⬆️
pandas/io/gbq.py	`78.94% <0%> (-10.53%)`	⬇️
pandas/core/dtypes/common.py	`96.62% <0%> (+0.34%)`	⬆️

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 891a419...39230d4. Read the comment docs.

codecov · 2019-06-06T04:26:46Z

Codecov Report

Merging #26684 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #26684      +/-   ##
==========================================
- Coverage   91.87%   91.86%   -0.01%     
==========================================
  Files         180      180              
  Lines       50712    50719       +7     
==========================================
+ Hits        46590    46593       +3     
- Misses       4122     4126       +4

Flag	Coverage Δ
#multiple	`90.46% <100%> (ø)`	⬆️
#single	`41.11% <66.66%> (-0.08%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/groupby/ops.py	`96% <100%> (ø)`	⬆️
pandas/core/frame.py	`96.89% <100%> (-0.12%)`	⬇️
pandas/core/generic.py	`94.2% <100%> (ø)`	⬆️
pandas/core/series.py	`93.66% <100%> (+0.01%)`	⬆️
pandas/io/gbq.py	`88.88% <0%> (-11.12%)`	⬇️

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 baa77c3...9f888c5. Read the comment docs.

jorisvandenbossche

Thanks for the PR!

Some additional comments:

There is an additional to_dense method in generic.py that should also be deprecated.
You will need to add some tests that those warnings are raised (as an example see the test added here: https://github.com/pandas-dev/pandas/pull/26421/files#diff-f446e63e2ec57a5d055add5511588c3fR1234)

pandas/core/frame.py

pandas/core/series.py

pandas/core/sparse/frame.py

VikramjeetD · 2019-06-07T11:45:00Z

@jorisvandenbossche Thanks for the review. I committed the stylistic changes that you suggested.

Working on writing the tests. Could you please guide me on where the tests for Series and Dataframe (not sparse) need to be written? Should I create a new file or write it in an existing file.

jreback · 2019-06-07T12:24:37Z

pandas/core/frame.py

@@ -1939,6 +1940,9 @@ def to_sparse(self, fill_value=None, kind='block'):
        >>> type(sdf)  # doctest: +SKIP
        <class 'pandas.core.sparse.frame.SparseDataFrame'>
        """
+        warnings.warn("DataFrame.to_sparse is deprecated and will be removed "
+                      "in a future version", FutureWarning, stacklevel=2)
+
        from pandas.core.sparse.api import SparseDataFrame
        return SparseDataFrame(self._series, index=self.index,


wont' this also trigger the SDF warnings? (should this just be changed to create a DF here?

Do you mean the SDF deprecation warning? If thats the case, yes, but this was requested in the issue that this PR will address ( #26557 ).
Also, this might be helpful if someone has already hit the SDF warning through a different route, as a gentle reminder. Either way we could decide whats best and update accordingly.

Yes, it already triggers the SparseDataFrame warning. But, it is still good to explicitly deprecate this method as well (in the docs, and by giving a clearer warning message, although the other one will also still be present).

I don't think we should change the behaviour, since we are deprecating it (it would also be a backwards incompatible change, as DataFrame with sparse and SparseDataFrame are not fully interchangeable).

So I think we can agree to keep both the warnings and resolve this conversation?

actually i would suppress the SDF
warning here as the user facing to_sparse is already good enough

pep8speaks · 2019-06-07T17:56:56Z

Hello @intell1gent! 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-06-18 04:51:35 UTC

VikramjeetD · 2019-06-07T18:28:42Z

...

There is an additional to_dense method in generic.py that should also be deprecated.
...

@jorisvandenbossche

Deprecating to_dense in generic.py breaks some tests for other classes. (I still haven't pushed that part). I could modify those tests but can't figure out how to ignore the FutureWarning. @pytest.mark.filterwarnings('ignore::FutureWarning') doesn't seem to help.
The other deprecations that I did raise a lot of warnings in many of the tests (but they pass)

Also, I'll change the code style issues in the next push.

jorisvandenbossche · 2019-06-08T05:34:21Z

@intell1gent I would need to look myself to say for sure (but now away for the weekend), but if other tests raise those warnings, then it might be because the methods are still used in certain places in pandas. In such a case, you will need to check those places and see if it can be replaced with something else (or if it was just used in the tests directly, then you can add a filterwarnings decorator).

VikramjeetD · 2019-06-08T06:17:43Z

@intell1gent I would need to look myself to say for sure (but now away for the weekend), but if other tests raise those warnings, then it might be because the methods are still used in certain places in pandas. In such a case, you will need to check those places and see if it can be replaced with something else (or if it was just used in the tests directly, then you can add a filterwarnings decorator).

Turns out changing UserWarning to Warning, check_stacklevel=False lets them pass.

pandas/tests/groupby/aggregate/test_aggregate.py

jreback

again, I don't want blanket warning filters at all. These are hiding errors.

pandas/core/sparse/frame.py

pandas/core/sparse/series.py

pandas/tests/arrays/sparse/test_arithmetics.py

pandas/tests/frame/test_deprecations.py

pandas/tests/generic/test_generic.py

jorisvandenbossche · 2019-06-17T11:48:45Z

again, I don't want blanket warning filters at all. These are hiding errors.

For the old sparse tests (so not speaking about the groupby tests, there we indeed should certainly not hide the warnings), we decided earlier to not rewrite those tests but just ignore the warnings (until all functionality + tests can be removed). So you will see there are already existing filterwarnings for Sparse. For those cases (again, tests about the old, deprecated SparseDataFrame/Series, I think it is best to also ignore those new warnings).

jorisvandenbossche · 2019-06-17T11:52:23Z

we still have to come to an unanimous decision whether to keep the double deprecation warning for Series/DataFrame.to_sparse. None of this will be needed if we deicide to stick with only 1 warning for the main deprecation.

@intell1gent I think it is fine to have the double deprecation. How would you propose to only have a single warning? I personally think we certainly need a warning that mentions to_sparse, so that would mean suppressing the other warning from inside to_sparse?

Update DataFrame/Series.to_dense deprecation message Co-Authored-By: Joris Van den Bossche <[email protected]>

jreback · 2019-06-17T11:54:00Z

again, I don't want blanket warning filters at all. These are hiding errors.

For the old sparse tests (so not speaking about the groupby tests, there we indeed should certainly not hide the warnings), we decided earlier to not rewrite those tests but just ignore the warnings (until all functionality + tests can be removed). So you will see there are already existing filterwarnings for Sparse. For those cases (again, tests about the old, deprecated SparseDataFrame/Series, I think it is best to also ignore those new warnings).

right this case is ok, but we need to be really clear on this, e.g. on pandas/tests/sparse should have these types of filters

jorisvandenbossche · 2019-06-17T11:55:05Z

Not added by me. Also present on upstream.

@intell1gent for future reference: you are correct that it was not added by you, but in general, the diff that github shows on the PR needs to be correct. So if you noticed unrelated changes in the diff here from master, that means that something went wrong when you updated this branch to the latest master. And it is still up to you to fix that. See my earlier comment on how that could be done.

jorisvandenbossche · 2019-06-17T11:59:06Z

right this case is ok, but we need to be really clear on this, e.g. on pandas/tests/sparse should have these types of filters

@jreback yes, that sounds correct, that we should in principle only do this general filterwarnings in the pandas/tests/sparse. However, it seems that with the original SparseDataFrame deprecation, such a general filterwarning was added in several other places as well (@TomAugspurger ? eg the pandas/tests/arrays/sparse/test_arithmetic.py do you remember why the general filterwarnings on Sparse was needed there?)

jreback · 2019-06-17T12:04:30Z

right this case is ok, but we need to be really clear on this, e.g. on pandas/tests/sparse should have these types of filters

@jreback yes, that sounds correct, that we should in principle only do this general filterwarnings in the pandas/tests/sparse. However, it seems that with the original SparseDataFrame deprecation, such a general filterwarning was added in several other places as well (@TomAugspurger ? eg the pandas/tests/arrays/sparse/test_arithmetic.py do you remember why the general filterwarnings on Sparse was needed there?)

right, let's comment these warning to be clear where they come from if they are not in pandas/tests/sparse (and actually maybe there too)

…ings

VikramjeetD · 2019-06-17T18:07:02Z

we still have to come to an unanimous decision whether to keep the double deprecation warning for Series/DataFrame.to_sparse. None of this will be needed if we deicide to stick with only 1 warning for the main deprecation.

@intell1gent I think it is fine to have the double deprecation. How would you propose to only have a single warning? I personally think we certainly need a warning that mentions to_sparse, so that would mean suppressing the other warning from inside to_sparse?

Yes, I changed to_sparse to ignore the SDF/SS warnings and only show a single warning. The main warning shows in other cases related to SDF/SS.

jorisvandenbossche

Looking good!

pandas/core/sparse/frame.py

pandas/tests/frame/test_deprecations.py

jorisvandenbossche · 2019-06-18T08:03:29Z

pandas/tests/sparse/frame/test_frame.py

+        with tm.assert_produces_warning(FutureWarning,
+                                        check_stacklevel=False):
+            result = df.to_sparse()
+        tm.assert_frame_equal(result, sparse_df)


you put this in a class where those warnings you want to check for are generally ignored. Does that work? (just wondering)

Maybe we can also put it at the end of the file as a function instead of method, to avoid possible concerns about that.

But since the tests are passing, this is probably fine ;)

This actually works fine, tm.assert_produces_warning doesn't seem to care about filterwarnings. Remove the warning from to_sparse and this test fails.

I wanted to keep both (test_dense_to_sparse and test_deprecated_dense_to_sparse) close for easy reference, thats all. Open to shifting it to the end though.

Thanks for checking, then that sounds good!

jorisvandenbossche

Thanks, looks good!

@jreback you're fine with the current filterwarnings? They are only added there were we already had a filterwarning for other sparse warnings as well

cc @TomAugspurger

jreback · 2019-06-18T10:17:40Z

i will review at some point again

jreback · 2019-06-19T00:41:11Z

thanks @intell1gent !

VikramjeetD added 4 commits June 5, 2019 19:36

Deprecate Series/Dataframe.to_dense/to_sparse()

a71737c

Update series.py

fc08e93

Beautify

82c713d

Beautify

39230d4

VikramjeetD mentioned this pull request Jun 6, 2019

Deprecate Series/Dataframe.to_dense/to_sparse #26668

Closed

2 tasks

jorisvandenbossche added Sparse Sparse Data Type Deprecate Functionality to remove in pandas labels Jun 7, 2019

jorisvandenbossche reviewed Jun 7, 2019

View reviewed changes

Beautify

1e7c0e8

jreback requested changes Jun 7, 2019

View reviewed changes

Update Deprecated SparseDF/Series tests

e68826c

VikramjeetD added 10 commits June 8, 2019 11:54

Beautify

20b8962

Deprecate NDFrame.to_dense

50d0534

Beautify

933162d

Silence test time deprecation warnings

dd1e6c2

Beautify

c7f27fd

Propose changes to certain tests

be14520

Propose changes to tests. IGNORE PREV COMMIT.

b12e447

Silence test time deprecation warnings

2d4de51

Add tests for Series/DataFrame.to_sparse

eede9b8

Beautify

0b08795

jreback requested changes Jun 10, 2019

View reviewed changes

pandas/tests/groupby/aggregate/test_aggregate.py Outdated Show resolved Hide resolved

Modify test time warning silence

5182a1f

jreback requested changes Jun 17, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master'

871ccff

Update pandas/core/generic.py

a8f6c56

Update DataFrame/Series.to_dense deprecation message Co-Authored-By: Joris Van den Bossche <[email protected]>

VikramjeetD added 6 commits June 17, 2019 17:39

Merge branch 'master' of https://github.com/IntEll1gent/pandas

72aaca5

Update test time warnings and rectify df/series.to_sparse double warn…

6a6e333

…ings

Update more test time warnings

4e67856

Update test warnings

4a3181b

Update test warnings

a546a89

Revert minor changes

5fdb2f8

jorisvandenbossche changed the title ~~Deprecate Series/Dataframe.to_dense/to_sparse~~ DEPR: Deprecate Series/Dataframe.to_dense/to_sparse Jun 17, 2019

jorisvandenbossche reviewed Jun 17, 2019

View reviewed changes

pandas/core/sparse/frame.py Outdated Show resolved Hide resolved

pandas/tests/frame/test_deprecations.py Outdated Show resolved Hide resolved

VikramjeetD added 3 commits June 18, 2019 09:57

Change location of Series/df.test_deprecated_to_sparse

a627828

Remove SDF/SS.to_dense depr:class already deprecated

3d36430

Add whatsnew entry

9f888c5

jorisvandenbossche reviewed Jun 18, 2019

View reviewed changes

jorisvandenbossche approved these changes Jun 18, 2019

View reviewed changes

jreback approved these changes Jun 19, 2019

View reviewed changes

jreback added this to the 0.25.0 milestone Jun 19, 2019

jsexauer mentioned this pull request Jun 19, 2019

DEPR: Clean up list of deprecations from prior versions #6581

Closed

1 task

jreback merged commit 376a05e into pandas-dev:master Jun 19, 2019

jreback mentioned this pull request Dec 1, 2019

DEPR: deprecations log for removed issues #13777

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEPR: Deprecate Series/Dataframe.to_dense/to_sparse #26684

DEPR: Deprecate Series/Dataframe.to_dense/to_sparse #26684

VikramjeetD commented Jun 6, 2019 •

edited

Loading

codecov bot commented Jun 6, 2019

codecov bot commented Jun 6, 2019 •

edited

Loading

jorisvandenbossche left a comment

VikramjeetD commented Jun 7, 2019 •

edited

Loading

jreback Jun 7, 2019

VikramjeetD Jun 7, 2019 •

edited

Loading

jorisvandenbossche Jun 7, 2019

VikramjeetD Jun 8, 2019

jreback Jun 8, 2019

pep8speaks commented Jun 7, 2019 •

edited

Loading

VikramjeetD commented Jun 7, 2019 •

edited

Loading

jorisvandenbossche commented Jun 8, 2019

VikramjeetD commented Jun 8, 2019 •

edited

Loading

jreback left a comment •

edited

Loading

jorisvandenbossche commented Jun 17, 2019

jorisvandenbossche commented Jun 17, 2019

jreback commented Jun 17, 2019

jorisvandenbossche commented Jun 17, 2019

jorisvandenbossche commented Jun 17, 2019

jreback commented Jun 17, 2019

VikramjeetD commented Jun 17, 2019 •

edited

Loading

jorisvandenbossche left a comment

jorisvandenbossche Jun 18, 2019

jorisvandenbossche Jun 18, 2019

VikramjeetD Jun 18, 2019

jorisvandenbossche Jun 18, 2019

jorisvandenbossche left a comment

jreback commented Jun 18, 2019

jreback commented Jun 19, 2019

DEPR: Deprecate Series/Dataframe.to_dense/to_sparse #26684

DEPR: Deprecate Series/Dataframe.to_dense/to_sparse #26684

Conversation

VikramjeetD commented Jun 6, 2019 • edited Loading

codecov bot commented Jun 6, 2019

Codecov Report

codecov bot commented Jun 6, 2019 • edited Loading

Codecov Report

jorisvandenbossche left a comment

Choose a reason for hiding this comment

VikramjeetD commented Jun 7, 2019 • edited Loading

Choose a reason for hiding this comment

VikramjeetD Jun 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Jun 7, 2019 • edited Loading

Comment last updated at 2019-06-18 04:51:35 UTC

VikramjeetD commented Jun 7, 2019 • edited Loading

jorisvandenbossche commented Jun 8, 2019

VikramjeetD commented Jun 8, 2019 • edited Loading

jreback left a comment • edited Loading

Choose a reason for hiding this comment

jorisvandenbossche commented Jun 17, 2019

jorisvandenbossche commented Jun 17, 2019

jreback commented Jun 17, 2019

jorisvandenbossche commented Jun 17, 2019

jorisvandenbossche commented Jun 17, 2019

jreback commented Jun 17, 2019

VikramjeetD commented Jun 17, 2019 • edited Loading

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche left a comment

Choose a reason for hiding this comment

jreback commented Jun 18, 2019

jreback commented Jun 19, 2019

VikramjeetD commented Jun 6, 2019 •

edited

Loading

codecov bot commented Jun 6, 2019 •

edited

Loading

VikramjeetD commented Jun 7, 2019 •

edited

Loading

VikramjeetD Jun 7, 2019 •

edited

Loading

pep8speaks commented Jun 7, 2019 •

edited

Loading

VikramjeetD commented Jun 7, 2019 •

edited

Loading

VikramjeetD commented Jun 8, 2019 •

edited

Loading

jreback left a comment •

edited

Loading

VikramjeetD commented Jun 17, 2019 •

edited

Loading