DEPR: deprecate default of skipna=False in infer_dtype #24050

h-vetinari · 2018-12-02T18:16:04Z

follows up on the changed in ENH: Add skipna parameter to infer_dtype #17066
tests adapted / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

I know that v.0.22 is generally not counted towards the "3 major releases between warning and deprecation" policy, but since this is a private method, I'm thinking it should be OK?

Would make life easier in a couple of places (especially in cleaning up some of the casting code).

pep8speaks · 2018-12-02T18:16:17Z

Hello @h-vetinari! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on January 04, 2019 at 11:14 Hours UTC

h-vetinari

Some comments on specific changes

pandas/conftest.py

pandas/core/algorithms.py

pandas/core/dtypes/cast.py

pandas/core/indexes/base.py

pandas/io/sql.py

pandas/tests/dtypes/test_inference.py

codecov · 2018-12-02T19:06:11Z

Codecov Report

Merging #24050 into master will decrease coverage by <.01%.
The diff coverage is 60%.

@@            Coverage Diff             @@
##           master   #24050      +/-   ##
==========================================
- Coverage   42.45%   42.44%   -0.01%     
==========================================
  Files         161      161              
  Lines       51561    51561              
==========================================
- Hits        21888    21886       -2     
- Misses      29673    29675       +2

Flag	Coverage Δ
#single	`42.44% <60%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/arrays/integer.py	`37.97% <0%> (ø)`	⬆️
pandas/core/dtypes/cast.py	`48.25% <0%> (-0.17%)`	⬇️
pandas/core/indexes/base.py	`55.93% <100%> (ø)`	⬆️
pandas/core/algorithms.py	`50.47% <100%> (ø)`	⬆️
pandas/core/dtypes/dtypes.py	`75.9% <0%> (-0.26%)`	⬇️

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 35b1702...2c9e2ea. Read the comment docs.

codecov · 2018-12-02T19:06:11Z

Codecov Report

Merging #24050 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #24050      +/-   ##
==========================================
+ Coverage   92.38%   92.38%   +<.01%     
==========================================
  Files         166      166              
  Lines       52490    52490              
==========================================
+ Hits        48493    48494       +1     
+ Misses       3997     3996       -1

Flag	Coverage Δ
#multiple	`90.81% <100%> (ø)`	⬆️
#single	`43.04% <0%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/reshape/merge.py	`94.26% <100%> (ø)`	⬆️
pandas/core/arrays/array_.py	`100% <100%> (ø)`	⬆️
pandas/util/testing.py	`88.09% <0%> (+0.09%)`	⬆️

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 6249355...03c46cd. Read the comment docs.

gfyoung · 2018-12-02T19:24:13Z

Hmm...you do have to try a little just to get to the function via Python, so I'm leaning towards ok, but not 100% certain though.

cc @jreback

jreback · 2018-12-03T01:16:22Z

I don't think this was ever deprecated. So not really possible to actually switch it now. If you want to add a deprecation warning if its not passed (e.g. make the default None) that would be ok.

h-vetinari · 2018-12-03T06:59:27Z

@jreback
The current docstring (from #17066) says "The default of False will be deprecated in a later version of pandas."

Also, it's a method in a module that's explicitly marked as private (big red box at https://pandas.pydata.org/pandas-docs/stable/api.html), so I wonder if deprecation is necessary at all?

pandas/io/stata.py

h-vetinari

@bashtage
Thanks for taking a look!

pandas/io/stata.py

bashtage · 2018-12-04T08:21:08Z

What happens when you have

df = pd.DataFrame([[None],[None]],dtype='object')

IIRC None is an N/A type.

pandas/_libs/lib.pyx

h-vetinari · 2018-12-04T18:58:50Z

@bashtage

What happens when you have
df = pd.DataFrame([[None],[None]],dtype='object')
IIRC None is an N/A type.

This also doesn't change between the two variants:

>>> import pandas as pd
>>> import pandas._libs.lib as lib
>>> df = pd.DataFrame([[None], [None]], dtype=object)
>>>
>>> lib.infer_dtype(df, skipna=True)
'integer'
>>> lib.infer_dtype(df.dropna(), skipna=False)
'integer'

That said, the result is a bit unexpected, and seems to be due the 2d case being handled differently. In any case, both variants (what you commented on, resp. what I'm changing it to) coincide also for 1d here:

>>> lib.infer_dtype(df.values.ravel(), skipna=True)
'empty'
>>> lib.infer_dtype(df.dropna().values.ravel(), skipna=False)
'empty'
>>>
>>> # and for comparison
>>> lib.infer_dtype(df.values.ravel(), skipna=False)
'mixed'

h-vetinari · 2018-12-12T19:14:39Z

@TomAugspurger

I implemented the DeprecationWarning like you requested - this should be ready.

pandas/_libs/lib.pyx

pandas/core/algorithms.py

pandas/core/dtypes/cast.py

pandas/core/dtypes/common.py

pandas/core/indexes/base.py

h-vetinari

Thanks for the review

pandas/_libs/lib.pyx

pandas/core/indexes/base.py

h-vetinari · 2018-12-19T23:43:06Z

@jreback Merged and green.

pandas/core/indexes/base.py

h-vetinari

@jreback
I think there's a misunderstanding here. Please have a look at my response and TAL.

pandas/core/indexes/base.py

pandas/_libs/lib.pyx

pandas/core/indexes/base.py

h-vetinari

@jreback
I have very limited capability over the holidays. Will most likely not be able to make a precursor before the cutoff. I could switch everything to skipna=False, and then things would definitely be the same as before.

pandas/_libs/lib.pyx

pandas/core/indexes/base.py

h-vetinari · 2019-01-02T22:42:07Z

@jreback
Split off a precursor as you asked in https://github.com/pandas-dev/pandas/pull/24050/files#r243769223 / https://github.com/pandas-dev/pandas/pull/24050/files#r243769260: #24560

h-vetinari · 2019-01-03T11:54:45Z

@jreback @TomAugspurger

Merged in #24560 and restored the state that was approved by @TomAugspurger (in 1ea21fe), which is all green now. The philosophy of that was to use skipna=True in as many places as possible, not least since the original approach of this PR was to set the default to True right away.

To avoid getting bogged down in discussing the implications before the cut-off, I've added the last commit, which maintains the current behaviour (skipna=False), and only adds the deprecation. The usages within tests are an exception to this, because they are ipso facto correct with a passing CI.

If changing the defaults in the codebase is desired, I can make a follow-up with 1ea21fe.

jreback

ok lgtm. very minor change. ping on green.

jreback · 2019-01-04T00:56:31Z

pandas/tests/dtypes/test_inference.py

        assert result == 'integer'

+    def test_warn(self):


rename to test_deprecation & add the issue number as a comment

h-vetinari · 2019-01-04T11:15:31Z

@jreback
Thanks. Any comment on whether you'd like me to transition the calls from skipna=False to skipna=True where possible (#24050 (comment))?

jreback · 2019-01-04T12:17:15Z

thanks @h-vetinari

I think we should only change to True in non-trivial cases, IOW the trivial ones can stay False; I think should be very explicit and deliberate for True. (so a lot of the string like ones this makes sense)

jorisvandenbossche · 2019-01-04T14:32:22Z

A bit late to the party here :-), but:

What is actually the rationale for this change?

I went quickly through the PR here but didn't directly find any reasoning apart from "Would make life easier in a couple of places (especially in cleaning up some of the casting code)." in the top post. Was this discussed in another issue?

Also to quote Jeff from his last comment just above: "IOW the trivial ones can stay False; I think should be very explicit and deliberate for True." (my emphasis)
If you want to be explicit in doing True, why then making this the default?

For the string case I think it certainly makes sense to have this as default, but for the integer case you now get a inconsistency between what infer_dtype tells you and what Series does: infer_dtype([1, 2, np.nan] == 'integer vs pd.Series([1, 2, np.nan]) == float64 (of course, if we ever have NA support by default for ints, this argument is no longer valid).

If we keep the warning (not really a strong opinion here, mainly wondering the rationale in the above comment): what do you all think of making this a DeprecationWarning instead of FutureWarning first, given that it is more a functions used by developers / other libraries and less directly by users?

h-vetinari · 2019-01-04T16:57:48Z

@jorisvandenbossche

What is actually the rationale for this change?

Actually, my main reason for deprecating was that it has been "announced" in the docstring since 0.21 (see diff of this PR):

The default of ``False`` will be deprecated in a later version of pandas.

That being said, I think the inference generally makes more sense when nans are ignored (just that historically, skipna was not available until #17066).

If we keep the warning (not really a strong opinion here, mainly wondering the rationale in the above comment): what do you all think of making this a DeprecationWarning instead of FutureWarning first, given that it is more a functions used by developers / other libraries and less directly by users?

I agree that it's not very user-facing. I have no strong preference for the type of warning.

)

DEPR: switch default of skipna-kwarg in infer_dtype to True

2c9e2ea

h-vetinari commented Dec 2, 2018

View reviewed changes

gfyoung added the Dtype Conversions Unexpected or buggy dtype conversions label Dec 2, 2018

h-vetinari mentioned this pull request Dec 3, 2018

ENH: coalesce-method (upgrade for update/combine_first) #22812

Open

bashtage reviewed Dec 3, 2018

View reviewed changes

pandas/io/stata.py Outdated Show resolved Hide resolved

h-vetinari commented Dec 4, 2018

View reviewed changes

pandas/io/stata.py Outdated Show resolved Hide resolved

TomAugspurger reviewed Dec 4, 2018

View reviewed changes

pandas/_libs/lib.pyx Show resolved Hide resolved

h-vetinari added 3 commits December 6, 2018 00:36

Merge remote-tracking branch 'upstream/master' into infer_skipna

9d2da42

Change default of skipna to None & warn; add whatsnew

21f7963

Missed one

568bfd9

jorisvandenbossche changed the title ~~DEPR: switch default of skipna-kwarg in infer_dtype to True~~ DEPR: deprecate default of skipna=False in infer_dtype Dec 6, 2018

jreback requested changes Dec 13, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into infer_skipna

4900f4b

h-vetinari commented Dec 14, 2018

View reviewed changes

pandas/_libs/lib.pyx Outdated Show resolved Hide resolved

pandas/core/indexes/base.py Outdated Show resolved Hide resolved

h-vetinari added 3 commits December 15, 2018 00:50

Review (jreback)

8c3c5b4

Merge remote-tracking branch 'upstream/master' into infer_skipna

698b0d0

Fix new occurrence of infer_dtype

52616b0

TomAugspurger approved these changes Dec 20, 2018

View reviewed changes

h-vetinari added 2 commits December 21, 2018 11:41

Merge remote-tracking branch 'upstream/master' into infer_skipna

d72dbda

Fix overlooked merge artefact

245b7bc

jreback requested changes Dec 21, 2018

View reviewed changes

pandas/core/indexes/base.py Outdated Show resolved Hide resolved

h-vetinari commented Dec 21, 2018

View reviewed changes

pandas/core/indexes/base.py Outdated Show resolved Hide resolved

jreback requested changes Dec 23, 2018

View reviewed changes

pandas/_libs/lib.pyx Outdated Show resolved Hide resolved

pandas/_libs/lib.pyx Outdated Show resolved Hide resolved

pandas/core/indexes/base.py Outdated Show resolved Hide resolved

h-vetinari commented Dec 24, 2018

View reviewed changes

pandas/_libs/lib.pyx Outdated Show resolved Hide resolved

pandas/core/indexes/base.py Outdated Show resolved Hide resolved

Merge remote-tracking branch 'upstream/master' into infer_skipna

4d97ce8

h-vetinari mentioned this pull request Jan 2, 2019

REF: shift ravel in infer_dtype #24560

Merged

h-vetinari added 6 commits January 3, 2019 10:52

Merge remote-tracking branch 'upstream/master' into infer_skipna

18e7bc6

Oversight

72ba292

Add kwarg to new call-sites

ebd1063

and another new call-site

e287bd8

Try skipna=True for new call-sites

1ea21fe

Revert skipna=True where it was added

fd8c010

jreback added this to the 0.24.0 milestone Jan 4, 2019

jreback requested changes Jan 4, 2019

View reviewed changes

jsexauer mentioned this pull request Jan 4, 2019

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

Closed

1 task

h-vetinari added 2 commits January 4, 2019 12:13

Review (jreback)

1edd0e9

Merge remote-tracking branch 'upstream/master' into infer_skipna

03c46cd

jreback approved these changes Jan 4, 2019

View reviewed changes

jreback merged commit 2f842f2 into pandas-dev:master Jan 4, 2019

h-vetinari deleted the infer_skipna branch January 4, 2019 16:58

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

DEPR: deprecate default of skipna=False in infer_dtype (pandas-dev#24050

0de9973

)

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

DEPR: deprecate default of skipna=False in infer_dtype (pandas-dev#24050

479b3bd

)

jreback mentioned this pull request Nov 28, 2019

DEPR: deprecations log for removed issues #13777

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEPR: deprecate default of skipna=False in infer_dtype #24050

DEPR: deprecate default of skipna=False in infer_dtype #24050

h-vetinari commented Dec 2, 2018 •

edited

Loading

pep8speaks commented Dec 2, 2018 •

edited

Loading

h-vetinari left a comment

codecov bot commented Dec 2, 2018

codecov bot commented Dec 2, 2018 •

edited

Loading

gfyoung commented Dec 2, 2018

jreback commented Dec 3, 2018

h-vetinari commented Dec 3, 2018

h-vetinari left a comment

bashtage commented Dec 4, 2018

h-vetinari commented Dec 4, 2018

h-vetinari commented Dec 12, 2018

h-vetinari left a comment

h-vetinari commented Dec 19, 2018

h-vetinari left a comment

h-vetinari left a comment

h-vetinari commented Jan 2, 2019

h-vetinari commented Jan 3, 2019 •

edited

Loading

jreback left a comment

jreback Jan 4, 2019

h-vetinari commented Jan 4, 2019

jreback commented Jan 4, 2019

jorisvandenbossche commented Jan 4, 2019

h-vetinari commented Jan 4, 2019

DEPR: deprecate default of skipna=False in infer_dtype #24050

DEPR: deprecate default of skipna=False in infer_dtype #24050

Conversation

h-vetinari commented Dec 2, 2018 • edited Loading

pep8speaks commented Dec 2, 2018 • edited Loading

Comment last updated on January 04, 2019 at 11:14 Hours UTC

h-vetinari left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 2, 2018

Codecov Report

codecov bot commented Dec 2, 2018 • edited Loading

Codecov Report

gfyoung commented Dec 2, 2018

jreback commented Dec 3, 2018

h-vetinari commented Dec 3, 2018

h-vetinari left a comment

Choose a reason for hiding this comment

bashtage commented Dec 4, 2018

h-vetinari commented Dec 4, 2018

h-vetinari commented Dec 12, 2018

h-vetinari left a comment

Choose a reason for hiding this comment

h-vetinari commented Dec 19, 2018

h-vetinari left a comment

Choose a reason for hiding this comment

h-vetinari left a comment

Choose a reason for hiding this comment

h-vetinari commented Jan 2, 2019

h-vetinari commented Jan 3, 2019 • edited Loading

jreback left a comment

Choose a reason for hiding this comment

jreback Jan 4, 2019

Choose a reason for hiding this comment

h-vetinari commented Jan 4, 2019

jreback commented Jan 4, 2019

jorisvandenbossche commented Jan 4, 2019

h-vetinari commented Jan 4, 2019

h-vetinari commented Dec 2, 2018 •

edited

Loading

pep8speaks commented Dec 2, 2018 •

edited

Loading

codecov bot commented Dec 2, 2018 •

edited

Loading

h-vetinari commented Jan 3, 2019 •

edited

Loading