DEPR: Deprecate ordered=None for CategoricalDtype
State: closed

- [X] closes #26336
- [X] tests added / passed
- [X] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [X] whatsnew entry

All of the logic related to breaking changes looks to be consolidated in `CategoricalDtype.update_dtype`, so I'm raising the warning there when a breaking change would occur and letting it flow through from there.

Aside from `CategoricalDtype.update_dtype`, I think the only other place where a breaking change can manifest is in `astype` as described in the issue.  Will need to double check the test logs to see if anything else triggered a warning though.

cc @TomAugspurger @jorisvandenbossche 


Labels:
name: Dtype Conversions
description: Unexpected or buggy dtype conversions
name: Categorical
description: Categorical Data Type
name: Deprecate
description: Functionality to remove in pandas

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26403?src=pr&el=h1) Report
> Merging [#26403](https://codecov.io/gh/pandas-dev/pandas/pull/26403?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/3b24fb678672ecb6ef24a1915c49f3cf815d5556?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/26403/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/26403?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #26403      +/-   ##
==========================================
- Coverage   91.69%   91.68%   -0.01%     
==========================================
  Files         174      174              
  Lines       50743    50746       +3     
==========================================
- Hits        46527    46526       -1     
- Misses       4216     4220       +4
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.19% <100%> (ø)` | :arrow_up: |
| #single | `41.17% <33.33%> (-0.16%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/26403?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/dtypes/dtypes.py](https://codecov.io/gh/pandas-dev/pandas/pull/26403/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2R0eXBlcy5weQ==) | `96.69% <100%> (+0.02%)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/26403/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `78.94% <0%> (-10.53%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/26403/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.02% <0%> (-0.12%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26403?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26403?src=pr&el=footer). Last update [3b24fb6...adc0bca](https://codecov.io/gh/pandas-dev/pandas/pull/26403?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26403?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@ddec4eb`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `28.57%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/26403/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/26403?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##             master   #26403   +/-   ##
=========================================
  Coverage          ?   41.69%           
=========================================
  Files             ?      174           
  Lines             ?    50760           
  Branches          ?        0           
=========================================
  Hits              ?    21162           
  Misses            ?    29598           
  Partials          ?        0
```

| Flag | Coverage Δ | |
|---|---|---|
| #single | `41.69% <28.57%> (?)` | |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/26403?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/packers.py](https://codecov.io/gh/pandas-dev/pandas/pull/26403/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3BhY2tlcnMucHk=) | `14.57% <0%> (ø)` | |
| [pandas/core/dtypes/dtypes.py](https://codecov.io/gh/pandas-dev/pandas/pull/26403/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2R0eXBlcy5weQ==) | `73.62% <33.33%> (ø)` | |
| [pandas/core/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/26403/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc2VyaWVzLnB5) | `45.76% <50%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26403?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26403?src=pr&el=footer). Last update [ddec4eb...bc16e98](https://codecov.io/gh/pandas-dev/pandas/pull/26403?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

> [about passing `ordered=None` explicitly] assuming we would disallow that in the future? 

Actually, we could still accept it but turn it into False?
One slight concern, In the future, `CategoricalDtype().ordered` will change without warning, right? Right now it's `None`, in the future it'll be `False`.

I'm not sure how we could easily raise a warning there (on attribute access) without being very annoying.
> One slight concern, In the future, CategoricalDtype().ordered will change without warning, right? Right now it's None, in the future it'll be False.

One option might to be already change it now? (to prevent more users to start relying on it) We could already return False when the user asks for it and internally None was stored?
> do we have any tests which are producing this warning that are not caught?

Yes, this is the reason CI failed on my first commit. The warning came from a case we weren't currently addressing, which I detailed in https://github.com/pandas-dev/pandas/issues/26336#issuecomment-494227301

> The one case that is not covered (which I mentioned in the issue) is when passing it manually to the constructor

We do this a lot internally in the codebase as well, so it seems like it'd be hard to do this is a non-invasive way that doesn't break things. Will look into this in the next couple days, likewise with `CategoricalDtype().ordered`.

can you merge master
Updated to address remaining comments from the issue, as well as align on comments from the sprint.

**Summary of the issue to refresh people's memory**
- Deprecated `CategoricalDtype` default value `ordered=None` in favor of `ordered=False`
- Still allow `ordered=None` if explicitly passed, in part to maintain behavior for the string `'category'`.

**Summary of impacted behavior**
- `astype` behavior could silently change since `ordered=None` can be used to maintain the existing `ordered` (xref https://github.com/pandas-dev/pandas/issues/26336#issuecomment-491447039)
- `Series` constructor behavior could silently change, as `ordered=None` can be used to maintain the existing ordered of a `Categorical` (xref https://github.com/pandas-dev/pandas/issues/26336#issuecomment-494227301)
- `CategoricalDtype.ordered` could silently change from `None` --> `False` if the default is changed to `False` (xref https://github.com/pandas-dev/pandas/pull/26403#issuecomment-492668006)
  - Should not impact `Categorical.ordered` or `CategoricalIndex.ordered` as `None` should be resolved to a boolean during construction.

**Implementation comments**
I tried to implement the changes in the least intrusive way; a warning will only be raised in scenarios where the existing behavior would silently change.

For example, with `CategoricalDtype.ordered`, the warning will only appear if no value for `None` is specified.  Note that constructing a `CategoricalDtype` without specifying `None` will not warn, as the behavior may not be silently changing at this point; only upon accessing `.ordered` will a warning appear.

```python
In [1]: from pandas.api.types import CategoricalDtype

In [2]: cdt = CategoricalDtype(['a', 'b'])

In [3]: cdt.ordered
/home/jeremy/code/pandas/pandas/core/dtypes/dtypes.py:589: FutureWarning: Constructing a
CategoricalDtype without specifying `ordered` will default to `ordered=False` in a future
version; `ordered=None` must be explicitly passed.
  warnings.warn(msg, FutureWarning)
```

Explicitly passing `None` will not warn when `.ordered` is accessed, since this behavior will still be allowed.  Likewise explicitly passing `True` or `False` will not warn.

```python
In [4]: CategoricalDtype(['a', 'b'], ordered=None).ordered is None
Out[4]: True

In [5]: CategoricalDtype(['a', 'b'], ordered=True).ordered
Out[5]: True

In [6]: CategoricalDtype(['a', 'b'], ordered=False).ordered
Out[6]: False
```
@jschendel Thanks for the detailed and clear summary! 
On board with the proposed changes and implementation details
Thanks for the summary in https://github.com/pandas-dev/pandas/pull/26403#issuecomment-507351745. That all sounds good, haven't had a chance to look at the changes yet. Probably won't today.
this looks fine. @jschendel can you rebase.
actually was able to use the conflict resolver. merge on green.