API: Use object dtype for empty Series
State: closed

- [x] closes #17261
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Labels:
name: Dtype Conversions
description: Unexpected or buggy dtype conversions
name: API Design
name: Series
description: Series data structure

Comments:
Meant to open a draft PR - please don't review this for now.

I assume quite a few more warnings and broken tests will need to be fixed
Mmm sorry. We need to have a discussion about whether this is worth the change.

At this point, I'm not sure...

This can be made backwards compatible, right? When the data is empty and no dtype is specified, we can emit a warning saying "pass dtype=float to keep the old behavior, or dtype=object for the new behavior."
> We need to have a discussion about whether this is worth the change.

Do you mean if we want to change it at all? No strong opinion about it, but it seems reasonable to want consistent behaviour across Series, DataFrame and Index for this. Any particular reason why we wouldn't want that?

> This can be made backwards compatible, right?

Why would we want to stay backwards compatible here? A big release like 1.0.0 seems like a pretty good opportunity to break this behaviour. We could obviously add this warning, but I wonder if it's really meaningful. Or do you mean we should add this warning before implementing the actual change, i.e. going through a full deprecation cycle?
@SaturnFromTitan we generally don’t like to non backward compatible changes if we can help it; or if not provide a deprecation period

this change would likely break lots of code that is out there so better to provide some notice before we actually change it
@jreback 
Gotcha. So should I indeed change the PR to just raise the warning instead then? Or is it up for debate whether we want to go in this direction at all?
yes only a warning for now
Agreed that a warning is the best path forward.
Phew... I changed the dtype change to a warning, but now realise this triggers an immense amount of warnings that clutter all test output. Silencing all of them feels like a huge undertaking... Is there an easier way around this @jreback @TomAugspurger?
They'll all need to be updated. We don't allow unhandled warnings in our
test suite.

We'll also need a new test ensuring that not specifying the dtype or data
results in a warning.

On Fri, Nov 8, 2019 at 4:59 PM Martin Winkel <notifications@github.com>
wrote:

> Phew... I changed the dtype change to a warning, but now realise this
> triggers an immense amount of warnings that clutter all test output.
> Silencing all of them feels like a huge undertaking... Is there an easier
> way around this @jreback <https://github.com/jreback> @TomAugspurger
> <https://github.com/TomAugspurger>?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/29405?email_source=notifications&email_token=AAKAOITNOSGAE5UEJR6BAVDQSXVLRA5CNFSM4JI3MAE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDTTKRY#issuecomment-552023367>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAKAOIVNPLWMGY64RGYRR63QSXVLRANCNFSM4JI3MAEQ>
> .
>

> Phew... I changed the dtype change to a warning, but now realise this triggers an immense amount of warnings that clutter all test output. Silencing all of them feels like a huge undertaking... Is there an easier way around this @jreback @TomAugspurger?

as tom indicates the best way here is to correct the expected in the tests
so there is no warning
@jreback @TomAugspurger I understand how I can resolve the warnings, the problem I see is the amount of work. This code change now raises >4000 warnings. Changing all those tests would not only require a lot of work but would also result in a gigantic PR :D

Now the question: Do you think there is a good way to work around this or should we rather close this PR as "Won't fix" because of the value it brings doesn't justify the effort it demands?
there shouldn’t be nearly this many warnings. the change is only for a very small subset of things, eg empty Series with no dtype

your patch is catching other cases; haven’t looked in depth but likely
you should look at a test which shouldn’t be changed and debug


@jreback Thanks for taking a look at the PR already! I think I still need to rework some of the changes and silence more warnings, but it's already very helpful.
Alright, I think this is ready for review now @jreback. Thanks!
@jreback I rebased again and resolved the failing checks for the documentation. Please have a look when you have time :)
@jreback @TomAugspurger I believe I addressed all your comments. Please take another look when you have the opportunity. Thanks!
@jreback Thanks for the review. I took care of all your comments.
@jreback @TomAugspurger I rebased and the build is green again. Please have a look when you get the chance. Thanks!
And @SaturnFromTitan, thanks a lot for working on this!
> Given that this can have some impact on libraries (see how often we used it ourselves in pandas), meaning that their users will start to see warnings they cannot solve, should be start with making this a DeprecationWarning instead of FutureWarning?
> 
> Given that this won't be actually changed in 1.0 (but only start raising a warning), we have some time to change it to a FutureWarning in a later release.

I guess we could have ``DeprecationWarning`` for a single version, then change to ``FutureWarning`` to give libraries a chance to change. I feel these are ignored as well by the downstream as they don't see them by-default (we in fact tend to ignore numpy ones :->)

@SaturnFromTitan let's change this and open an issue for the 1.1 milestone (let me create it) to change to a FutureWarning
@jreback @TomAugspurger @jorisvandenbossche Thanks for your comments! I think I addressed all of them. Please let me know if you think there's more to do.

Note: CI is failing, but that seems unrelated.
Looks all good to me now! 
CI failures are indeed unrelated.

@SaturnFromTitan for future reference: if you could push only new commits instead of squashing with previous commits (and thus force pushing), that would be preferred. It makes it easier to see what has changes since a previous review with the github interface.
> we didn’t actually adopt this

@jreback it's what is written in https://dev.pandas.io/docs/development/policies.html (https://github.com/pandas-dev/pandas/pull/28415/). If you want to reopen that discussion (or seek for further clarifications), maybe start a mailing list discussion.
This is green as well @jreback 
I'm not too sure about the types I assigned for `create_series_with_explicit_dtype` since `Series` itself isn't typed @jreback @jorisvandenbossche 
@jreback merge conflicts resolved and CI is green ✅ 
thanks @SaturnFromTitan 

great patch! this was a biggie.

do we have an issue to turn this into a FutureWarning? 

happy to accept patches for things like typing (or anything else)!
Yes, here's the issue for turning it into a FutureWarning: #30017