DEPR/CLN: Clean up to_dense signature deprecations #22910

gfyoung · 2018-09-30T18:43:56Z

Also deprecate fill in .get_values because its parameter was being passed to .to_dense, which
no longer accepts the fill parameter.

xref #14686.

pep8speaks · 2018-09-30T18:44:01Z

Hello @gfyoung! Thanks for submitting the PR.

There are no PEP8 issues in the file pandas/core/sparse/array.py !
There are no PEP8 issues in the file pandas/core/sparse/series.py !
There are no PEP8 issues in the file pandas/tests/sparse/series/test_series.py !
There are no PEP8 issues in the file pandas/tests/sparse/test_array.py !

codecov · 2018-09-30T23:42:31Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #22910      +/-   ##
==========================================
- Coverage   92.19%   92.18%   -0.01%     
==========================================
  Files         169      169              
  Lines       50954    50946       -8     
==========================================
- Hits        46975    46967       -8     
  Misses       3979     3979

Flag	Coverage Δ
#multiple	`90.61% <100%> (-0.01%)`	⬇️
#single	`42.28% <75%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/arrays/sparse.py	`92.52% <100%> (-0.03%)`	⬇️
pandas/core/sparse/series.py	`95.49% <100%> (-0.1%)`	⬇️

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 1546c35...bfbae5c. Read the comment docs.

jreback · 2018-10-01T11:48:31Z

hmm, this might need to go in after @TomAugspurger refactor of Sparse (#22325)

gfyoung · 2018-10-01T16:20:01Z

Sounds good. I'll wait until that's done then.

gfyoung · 2018-10-16T20:45:02Z

Looks like Azure is failing on master but could be addressed by #23182.

TomAugspurger · 2018-10-16T20:52:06Z

I'm fine with ignoring these failures for now if the code isn't touching window / pyarrow.

gfyoung · 2018-10-16T21:41:06Z

@TomAugspurger : These changes don't touch either of those. That being said, I'm in no rush to merge these, and I see that you are getting close on #23182.

Unless somebody needs these deprecations removed NOW, I think I can hold.

jorisvandenbossche · 2018-10-16T21:44:00Z

I think we could deprecate get_values all together? (instead of only deprecating the keyword)

gfyoung · 2018-10-16T21:50:36Z

I think we could deprecate get_values all together? (instead of only deprecating the keyword)

@jorisvandenbossche :

Ah, hence the TODO in the original code. I think to_dense makes more sense semantically, so I don't see why we don't do that. If there aren't any objections, I can do that as well in this PR.

jreback · 2018-10-16T22:01:56Z

i agree we should deprecate get_values fully
but that means changing where it is used internally

i tried this a while back and it’s not so easy

maybe easier now that SpareArray is here

jorisvandenbossche · 2018-10-16T22:04:23Z

"in principle" it should not be used internally (outside of sparse itself), as it is not part of the EA interface (or where it is used in sparse specific code, it should be replacable with to_dense)
but, don't know to what extent this "in principle" is true :)

I think it is certainly worth to check in this PR.

gfyoung · 2018-10-16T23:59:31Z

I didn't follow the sparse refactoring too closely, but to what extent are we trying to have a consistent interface for SparseArray with respect to SparseSeries and SparseDataFrame ?

I ask this because although I'm also inclined to deprecate get_values, it is of course a completely valid method for the latter two. The devil's advocate in me thinks then that the interface for sparse object methods becomes a little inconsistent if you have to use to_dense for one class but are allowed to use get_values for the others.

jreback · 2018-10-17T01:06:25Z

this looks fine. can you rebase to get this passing. ping on green.

jreback · 2018-10-17T01:06:54Z

I think we have an issue about deprecating .get_values() generally, which as I said above we should do (but slightly tricky).

gfyoung · 2018-10-17T01:23:48Z

this looks fine. can you rebase to get this passing. ping on green.

@jreback : I'm waiting on #23182 to be merged.

jreback · 2018-10-17T01:32:16Z

@gfyoung yep ok.

jorisvandenbossche · 2018-10-17T12:04:35Z

to what extent are we trying to have a consistent interface for SparseArray with respect to SparseSeries and SparseDataFrame ?

There does not need to be a consistency here, since we are considering deprecating SparseSeries/DataFrame alltogether.

I still think it doesn't make sense to only deprecate the keyword if we are actually planning to deprecate the full method. I would directly do that, otherwise you annoy users twice for nothing.

TomAugspurger · 2018-10-17T12:15:45Z

FYI, I've started a exploring a deprecation for SparseDataFrame / SparseSeries. I think it'll be doable for 0.24.

…

On Wed, Oct 17, 2018 at 7:04 AM Joris Van den Bossche < ***@***.***> wrote: to what extent are we trying to have a consistent interface for SparseArray with respect to SparseSeries and SparseDataFrame ? There does not need to be a consistency here, since we are considering deprecating SparseSeries/DataFrame alltogether. I still think it doesn't make sense to only deprecate the feature if we are actually planning to deprecate the full method. I would directly do that, otherwise you annoy users twice for nothing. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#22910 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIuZgUn9b-20IRUSOIMMwCg4Xz1oKks5ulxzegaJpZM4XBGN8> .

gfyoung · 2018-10-17T20:55:12Z

As @jreback had warned beforehand, there are unfortunately still some issues with trying to deprecate to_dense at the moment. I would prefer to attempt to do this in a fresh PR once this is merged.

jorisvandenbossche · 2018-10-17T21:01:10Z

I think it was about get_values and not is_dense (maybe a typo)?

But, can you give a bit more details about the issues you encountered? That's useful information to keep track of.

gfyoung · 2018-10-17T22:14:47Z

@jorisvandenbossche : There were some errors in the sparse index testing. I'll try to debug as a follow-up.

gfyoung · 2018-10-18T08:09:52Z

@jreback : Addressed all comments, and all is green again! PTAL.

jorisvandenbossche · 2018-10-18T09:10:26Z

@gfyoung was looking at the diff now, and was wondering: can't you simply remove the fill keyword in get_values as well instead of deprecating it? Because you already got a deprecation warning before, since this was simply passed to to_dense, which raised the future warning.

gfyoung · 2018-10-18T10:03:00Z

@jorisvandenbossche : Ah! That's a good point! Let me do that then (given the warning message was not specific to either method).

Also deprecate `fill` in `.get_values` because its parameter was being passed to `.to_dense`, which no longer accepts the `fill` parameter. xref pandas-devgh-14686.

gfyoung · 2018-10-18T16:59:32Z

@jreback @jorisvandenbossche : Comments addressed, and all is green! PTAL.

jreback · 2018-10-18T17:30:07Z

thanks @gfyoung

Also deprecate `fill` in `.get_values` because its parameter was being passed to `.to_dense`, which no longer accepts the `fill` parameter. xref pandas-devgh-14686.

gfyoung added Deprecate Functionality to remove in pandas Clean labels Sep 30, 2018

gfyoung added this to the 0.24.0 milestone Sep 30, 2018

jreback mentioned this pull request Sep 30, 2018

DEPR: deprecations log for removed issues #13777

Closed

gfyoung force-pushed the to-dense-depr-remove branch from 21de4f4 to fa36970 Compare October 16, 2018 19:32

gfyoung force-pushed the to-dense-depr-remove branch from fa36970 to 2d1cebf Compare October 18, 2018 02:56

DEPR/CLN: Clean up to_dense signature deprecations

bfbae5c

Also deprecate `fill` in `.get_values` because its parameter was being passed to `.to_dense`, which no longer accepts the `fill` parameter. xref pandas-devgh-14686.

gfyoung force-pushed the to-dense-depr-remove branch 2 times, most recently from 624568a to bfbae5c Compare October 18, 2018 10:09

jreback merged commit 8963218 into pandas-dev:master Oct 18, 2018

gfyoung deleted the to-dense-depr-remove branch October 18, 2018 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEPR/CLN: Clean up to_dense signature deprecations #22910

DEPR/CLN: Clean up to_dense signature deprecations #22910

gfyoung commented Sep 30, 2018

pep8speaks commented Sep 30, 2018

codecov bot commented Sep 30, 2018 •

edited

Loading

jreback commented Oct 1, 2018 •

edited by gfyoung

Loading

gfyoung commented Oct 1, 2018

gfyoung commented Oct 16, 2018

TomAugspurger commented Oct 16, 2018

gfyoung commented Oct 16, 2018 •

edited

Loading

jorisvandenbossche commented Oct 16, 2018

gfyoung commented Oct 16, 2018 •

edited

Loading

jreback commented Oct 16, 2018

jorisvandenbossche commented Oct 16, 2018

gfyoung commented Oct 16, 2018 •

edited

Loading

jreback commented Oct 17, 2018

jreback commented Oct 17, 2018

gfyoung commented Oct 17, 2018

jreback commented Oct 17, 2018

jorisvandenbossche commented Oct 17, 2018 •

edited

Loading

TomAugspurger commented Oct 17, 2018 via email

gfyoung commented Oct 17, 2018 •

edited

Loading

jorisvandenbossche commented Oct 17, 2018 •

edited

Loading

gfyoung commented Oct 17, 2018 •

edited

Loading

gfyoung commented Oct 18, 2018

jorisvandenbossche commented Oct 18, 2018

gfyoung commented Oct 18, 2018 •

edited

Loading

gfyoung commented Oct 18, 2018

jreback commented Oct 18, 2018

DEPR/CLN: Clean up to_dense signature deprecations #22910

DEPR/CLN: Clean up to_dense signature deprecations #22910

Conversation

gfyoung commented Sep 30, 2018

pep8speaks commented Sep 30, 2018

codecov bot commented Sep 30, 2018 • edited Loading

Codecov Report

jreback commented Oct 1, 2018 • edited by gfyoung Loading

gfyoung commented Oct 1, 2018

gfyoung commented Oct 16, 2018

TomAugspurger commented Oct 16, 2018

gfyoung commented Oct 16, 2018 • edited Loading

jorisvandenbossche commented Oct 16, 2018

gfyoung commented Oct 16, 2018 • edited Loading

jreback commented Oct 16, 2018

jorisvandenbossche commented Oct 16, 2018

gfyoung commented Oct 16, 2018 • edited Loading

jreback commented Oct 17, 2018

jreback commented Oct 17, 2018

gfyoung commented Oct 17, 2018

jreback commented Oct 17, 2018

jorisvandenbossche commented Oct 17, 2018 • edited Loading

TomAugspurger commented Oct 17, 2018 via email

gfyoung commented Oct 17, 2018 • edited Loading

jorisvandenbossche commented Oct 17, 2018 • edited Loading

gfyoung commented Oct 17, 2018 • edited Loading

gfyoung commented Oct 18, 2018

jorisvandenbossche commented Oct 18, 2018

gfyoung commented Oct 18, 2018 • edited Loading

gfyoung commented Oct 18, 2018

jreback commented Oct 18, 2018

codecov bot commented Sep 30, 2018 •

edited

Loading

jreback commented Oct 1, 2018 •

edited by gfyoung

Loading

gfyoung commented Oct 16, 2018 •

edited

Loading

gfyoung commented Oct 16, 2018 •

edited

Loading

gfyoung commented Oct 16, 2018 •

edited

Loading

jorisvandenbossche commented Oct 17, 2018 •

edited

Loading

gfyoung commented Oct 17, 2018 •

edited

Loading

jorisvandenbossche commented Oct 17, 2018 •

edited

Loading

gfyoung commented Oct 17, 2018 •

edited

Loading

gfyoung commented Oct 18, 2018 •

edited

Loading