Fix type annotation in pandas.compat.numpy.function
State: closed

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


Labels:
name: Typing
description: type annotations, mypy/pyright type checking

Comments:
@WillAyd I think this is my third open PR for typing. Should I wait for these to get reviewed before creating new issues and PR?
Fine by me - thanks for all of the help so far!

Sent from my iPhone

> On Apr 2, 2019, at 7:29 AM, Vaibhav Vishal <notifications@github.com> wrote:
> 
> @WillAyd I think this is my third open PR for typing. Should I wait for these to get reviewed before creating new issues and PR?
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.

Ok, I will wait for at least one of them to get merged before working on new issues.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25961?src=pr&el=h1) Report
> Merging [#25961](https://codecov.io/gh/pandas-dev/pandas/pull/25961?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/00c119c50728bee88a5e5d1538cf3a405e36997c?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/25961/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/25961?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #25961      +/-   ##
==========================================
- Coverage   91.82%   91.81%   -0.01%     
==========================================
  Files         175      175              
  Lines       52581    52582       +1     
==========================================
- Hits        48280    48277       -3     
- Misses       4301     4305       +4
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.36% <100%> (ø)` | :arrow_up: |
| #single | `41.89% <100%> (-0.08%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25961?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/compat/numpy/function.py](https://codecov.io/gh/pandas-dev/pandas/pull/25961/diff?src=pr&el=tree#diff-cGFuZGFzL2NvbXBhdC9udW1weS9mdW5jdGlvbi5weQ==) | `87.97% <100%> (+0.06%)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/25961/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `75% <0%> (-12.5%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/25961/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `96.79% <0%> (-0.12%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25961?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/25961?src=pr&el=footer). Last update [00c119c...b02dfaf](https://codecov.io/gh/pandas-dev/pandas/pull/25961?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/25961?src=pr&el=h1) Report
> Merging [#25961](https://codecov.io/gh/pandas-dev/pandas/pull/25961?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/444ba646944c6a44490633587fc779d39b55cbfc?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/25961/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/25961?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #25961      +/-   ##
==========================================
- Coverage   91.98%   91.98%   -0.01%     
==========================================
  Files         175      175              
  Lines       52375    52376       +1     
==========================================
- Hits        48179    48176       -3     
- Misses       4196     4200       +4
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.53% <100%> (ø)` | :arrow_up: |
| #single | `40.7% <100%> (-0.15%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25961?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/compat/numpy/function.py](https://codecov.io/gh/pandas-dev/pandas/pull/25961/diff?src=pr&el=tree#diff-cGFuZGFzL2NvbXBhdC9udW1weS9mdW5jdGlvbi5weQ==) | `87.97% <100%> (+0.06%)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/25961/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/25961/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `96.9% <0%> (-0.12%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25961?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/25961?src=pr&el=footer). Last update [444ba64...a647e6f](https://codecov.io/gh/pandas-dev/pandas/pull/25961?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

IMO, these type hints in this PR are a little over-fitted to this file.  These default argument dictionaries are meant to map strings to anything that `numpy` throws at us from its signatures upstream.  It would be more prudent to actually declare these types as `OrderedDict[str, Any]` to indicate this.
Yea was thinking the same thing, though I think maybe wrapping the containers in Final would be a good indication that these aren't meant to be modified:

https://mypy.readthedocs.io/en/latest/final_attrs.html?highlight=immutable#details-of-using-final

> though I think maybe wrapping the containers in Final would be a good indication that these aren't meant to be modified

That's a fair proposition.
My bad - fat fingers 😂 
@WillAyd @gfyoung does this looks good:

    CLIP_DEFAULTS = dict(out=None)  # type Final[Dict[str, Any]]
@vaibhavhrt : That looks good!

If you could make similar modifications to your other changes, that would be great.
@gfyoung think should convert dict to OrderedDict too? Not sure if a distinction between those is intentional so maybe make consistent?
@WillAyd `Final` needs to be imported from `typing_extensions`(https://pypi.org/project/typing-extensions/). But I don't think we have that in requirements. I usually use pipenv so I am not sure how to add a new package to requirements in this project. Would it be enough to add `typing_extensions` to `requirements-dev.txt`
Well that's disappointing...not sure we want to add a third party library just yet. Was that not added until 3.7
>mypy pandas/compat/numpy/function.py

runs without any error but at `from typing_extensions import Final` my linter says `unresolved import 'typing_extensions' Python(unresolved-import)`. I am guessing tests might fail.
> think should convert dict to OrderedDict too? Not sure if a distinction between those is intentional so maybe make consistent?

@WillAyd : `OrderedDict` is used when there are multiple arguments (we need to maintain input order in the same way that `numpy` would define those arguments in its signature).  For single-argument compatibility, `dict` or `OrderedDict` should fine, though the latter might have some slight overhead.
@jreback do you have any thoughts on adding `typing_extensions` as a dependency to pandas? 

https://github.com/python/typing/tree/master/typing_extensions

On the plus side maintained by Guido and could help smooth over compat issues leading up to 3.7 (where typing is no longer provisional). On the downside would be another dependency

From there I think we could use Final, ChainMap, OrderedDict, Type, etc...

Side note - I know we've used Type in a few places already but reading more up on it that wasn't added until 3.5.2 so I think that might have broken the code base for 3.5 and 3.5.1
not a big deal to add typing_extensions

it’s pretty common now-a-days

let’s do that in a separate patch
Let me know when typing_extension has been added. @WillAyd if you want I can create an issue and PR to add typing_extensions to dependencies. But I am not sure how you are handling dependencies so  so I might need some help.
Sure give it a go! You should be able to see the PR to add Mypy which you can use for reference
@vaibhavhrt can you merge master
@jbrockmendel ok I will merge master. I have few more commits to push in this PR. But before I do that #25975 needs to be merged.  The dependency `typing_extensions` is needed for this PR.  But that PR is having some problems with CI configuration. Can you take a look and tell me how to fix that.
@vaibhavhrt we won't be merging that PR mentioned https://github.com/pandas-dev/pandas/pull/25961#issuecomment-481106021 as is, see the comments
Yeah I see. If no one is already working on adding required types to
pandas.typing I can do that in a separate  issue and PR.

On Wed, 17 Apr 2019, 1:09 am Jeff Reback, <notifications@github.com> wrote:

> @vaibhavhrt <https://github.com/vaibhavhrt> we won't be merging that PR
> mentioned #25961 (comment)
> <https://github.com/pandas-dev/pandas/pull/25961#issuecomment-481106021>
> as is, see the comments
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/25961#issuecomment-483814464>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AViWLPKUBRThNMUvhH2OgPl3dyLXdrNaks5vhibsgaJpZM4cYL4Y>
> .
>

Yes that’s correct. 

Sent from my iPhone

> On Apr 23, 2019, at 10:44 PM, Vaibhav Vishal <notifications@github.com> wrote:
> 
> @vaibhavhrt commented on this pull request.
> 
> In pandas/compat/numpy/function.py:
> 
> >  validate_round = CompatValidator(ROUND_DEFAULTS, fname='round',
>                                   method='both', max_fname_arg_count=1)
>  
> -SORT_DEFAULTS = OrderedDict()
> +SORT_DEFAULTS = OrderedDict()  # type: OrderedDict[str, Union[int, str, None]]
> @WillAyd , Optional takes only one argument, so instead of
> 
> Union[X, Y, None]
> Can I use optional like this 👇
> 
> Optional[Union[X, Y]]
> I am a bit busy on weekdays, I will fix it if I get some time, otherwise it will be on saturday(friday for you), sorry about that.
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.

Hello @vaibhavhrt! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:



There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2019-04-25 06:44:26 UTC
@WillAyd two tests are failing, but only thing I did was add comments, how can it make tests fail.
@jreback I have fixed everything, you can review and merge it.
thanks!